Conversation
| - describing the age of individual host entries | ||
| - annotating entries (descriptions of why a particular selector is needed, etc.) | ||
| - form rendering timings | ||
| - distinguishing URLs by query string and/or fragment that affect rendered form content | ||
| - describing dynamically-renamed fields (e.g. sites that randomize attribute values on each render) | ||
| - form inputs that represent user confirmation (e.g. agree to terms of service, etc) | ||
| - indicators of irrelevant data at the form field level |
There was a problem hiding this comment.
Some of these limitations are addressable, but require a level of consideration that would expand the scope of this effort considerably. We should identify the level of impact of these (and identify other cases) in our testing.
|
Checkmarx and Sonar scans are expected to fail as we get the repo configured. |
MGibson1
left a comment
There was a problem hiding this comment.
Looking really good. There are a few items in here that I think need to change before we can commit to schemas.
| if (host.startsWith("www.")) { | ||
| console.warn( | ||
| `\x1b[33mWarning: ${file} - host key "${host}" uses a www. prefix. ` + | ||
| `Author under the non-www host as canonical unless hosts differ. ` + |
There was a problem hiding this comment.
This line is unnecessarily terse to the point of being unclear
| `Author under the non-www host as canonical unless hosts differ. ` + | |
| `Prefer adding maps without the "www." prefix, unless rules differ between the "www." and un-prefixed. domains` + |
|
|
||
| const validate = ajv.compile(schema); | ||
| if (!validate(data)) { | ||
| console.error(`\x1b[31mValidation failed: ${file}\x1b[0m`); |
There was a problem hiding this comment.
It wouldn't be that big a deal to pick up chalk, would it?
There was a problem hiding this comment.
Another dependency is another attack vector 😭 😂
| using CSS selectors. It enables consuming applications to locate specific fields | ||
| without relying on heuristic determinations or page-specific detection logic. | ||
|
|
||
| This Map describes **the content of a page**. It does not prescribe or imply how |
There was a problem hiding this comment.
| This Map describes **the content of a page**. It does not prescribe or imply how | |
| This Map describes **the semantic content of a page**. It does not prescribe or imply how |
There was a problem hiding this comment.
I'm worried this term is overloaded for this context; we indeed describe non-semantic elements (e.g. div) to give them semantic meaning they otherwise don't have. What I was trying to get at with the emphasis here was that we're not, for example, interested in describing page interactions or programmatic code behaviours (at least not in this Map at this time)
edit: The revised form properties (93e3152) now describe form actions as an independent concern.
There was a problem hiding this comment.
Maybe this is obvious to a consumer, but the lift that semantic is doing in that suggestion isn't to distinguish from actions, but to make it clear that this map does not describe content directly, just the meaning behind whatever content is presented.
Pretty pedantic, I don't think I'm too fussed about this one.
There was a problem hiding this comment.
Not a gigantic pedantic semantic, granted
|
|
||
| This Map describes **the content of a page**. It does not prescribe or imply how | ||
| a consumer of this Map should behave. Additionally, the term "form" here | ||
| describes the user-facing concept, which may or may not utilize the HTML `form` |
There was a problem hiding this comment.
Describe the concept for clarity. I took a stab at what I suspect you mean. Surprisingly hard to describe tersely.
| describes the user-facing concept, which may or may not utilize the HTML `form` | |
| describes the user-facing concept of users supplying data to a web site, which may or may not utilize the HTML `form` |
There was a problem hiding this comment.
I think this suggestion works; the form element spec talks about "palpable content", which is an interesting idea in the mix here.
| ## Pathnames | ||
|
|
||
| The `pathnames` object maps URL pathnames to page-specific entries. Pathnames | ||
| must start with `/`. |
There was a problem hiding this comment.
It's captured in the schema (which we run validation against) as a regex pattern here:
https://github.com/bitwarden/map-the-web/pull/4/changes#diff-1d367c40e031794e17463f106698bdc17b6b8ce8127cc0d3b14b50d5dcb5085cR72-R73
| When `category` is omitted, the form's purpose is unspecified. Consumers should | ||
| not infer purpose from the absence of a category. |
There was a problem hiding this comment.
This should be unallowed in the schema, without a category, you are missing critical semantic information. As you point out, it renders data unusable, so why have it?
There was a problem hiding this comment.
As you point out, it renders data unusable
Perhaps I'm misunderstanding, but It does not; it optionally enriches it. The purpose of a form lacking the contextual information of category can still be inferred by the input values grouped by the form concept (which is what our consumer already does heuristically).
There was a problem hiding this comment.
to me it sounds like you're saying "don't infer the purpose of a form field" and then followed it up with "we do".
While I understand that you're saying you're pulling in contextual information around the field to infer it's purpose, that is not the purpose of these maps. That's describing our normal autofill process. I don't see value in a field in the map having no category, since the reason for the map to exist to the specify the purpose of a field.
There was a problem hiding this comment.
Ah, what I'm after is that the category absence itself does not have meaning (but consumers can still infer categories/context from the fields).
But perhaps it should not be optional. My intent was to cover categories we've not yet captured, but we probably want to capture them as we go. We can always revisit it later.
There was a problem hiding this comment.
addressed in 692d32e (category is no longer optional)
| The `selectors` object maps keys to arrays of CSS selectors. Each key is an | ||
| opaque identifier whose meaning is defined by the consuming application. The Map |
There was a problem hiding this comment.
Each key is an opaque identifier
I don't think this is going to fly, I understand it's hard to meaningfully list all options, but we need a defined set of known meanings and maybe support arbitrary data.
As written, the repository could become weird infighting where each application repeats work to appease some esoteric internal concern.
There was a problem hiding this comment.
Yeah, totally - as I called out in the PR summary; I intentionally deferred this, but it needs to be finalized before a release (of the project). Our consumers aren't currently making use of it, so I wanted to get a sense of the practical value add of this before committing to it.
There was a problem hiding this comment.
Thinking about this more, I see no reason we cannot solidify what we have now. We can always extend the schema during evaluation and have the ability to version the schema after release if needed.
I'll update the requirements and definitions of the selectors.
|
|
||
| The boundary type is determined by the element matched before `>>>`: | ||
|
|
||
| - **No `iframe` tag** in the preceding segment → shadow DOM boundary |
There was a problem hiding this comment.
And if there is no preceding segment? (this is probably not possible, you'd just use html, but idk, just noticed a potentially missing arm)
There was a problem hiding this comment.
I believe a host node will be required for both cases (iframe and shadow host). If the shadow root is at the root of the page somehow, yeah I'd expect it to be represented as html >>> nodetag - so for the purposes of the doc; we'd always expect something preceding (and following!) the >>> combinator (never naked). I'll clarify this point in the doc.
There was a problem hiding this comment.
| manifest.maps[map.name] = { | ||
| version: map.data.version, | ||
| files: [relative(DIST, outDataFile), relative(DIST, outSchemaFile)], | ||
| }; |
There was a problem hiding this comment.
This manifest schema will not work once you have to publish multiple versions of the same map, which I believe is a backward compatibility goal.
| - [Null and Empty Semantics](#null-and-empty-semantics) | ||
| - [Authoring Guidelines](#authoring-guidelines) | ||
|
|
||
| ## Limitations |
There was a problem hiding this comment.
Not to be conflated with goals - some of these are limitations we actively do want to eliminate, but may require additional work in follow up tasks; others are constraints of necessity
| using CSS selectors. It enables consuming applications to locate specific fields | ||
| without relying on heuristic determinations or page-specific detection logic. | ||
|
|
||
| This Map describes **the content of a page**. It does not prescribe or imply how |
There was a problem hiding this comment.
Maybe this is obvious to a consumer, but the lift that semantic is doing in that suggestion isn't to distinguish from actions, but to make it clear that this map does not describe content directly, just the meaning behind whatever content is presented.
Pretty pedantic, I don't think I'm too fussed about this one.
|
|
||
| ## Data Structure Overview | ||
|
|
||
| ```json |
There was a problem hiding this comment.
changing to jsonc may fix this red formtting of the comment. Probably not the ellipse, though
| } | ||
| ``` | ||
|
|
||
| ### Field Keys |
There was a problem hiding this comment.
Does adding a field key constitute a schema version bump? What about removing? Renaming?
I think you should define some process here since it's likely to occur. We can head off some confusion by thinking through it now and preparing.
| > [!IMPORTANT] | ||
| > A combined expiration field (`cardExpirationDate`) is not the same as separate | ||
| > month and year fields (`cardExpirationMonth` / `cardExpirationYear`). Use the | ||
| > key that matches the actual input structure on the page. |
There was a problem hiding this comment.
nit: different structure to this callout vs address, phone, birthday, etc
| Order is significant within a sequence. The map does not specify how the value | ||
| is split across the elements. |
There was a problem hiding this comment.
Again, it feels like it does and needs to. Looking back at the name, birthday, etc, all of those define where and how to split data. This isn't really a special case.
I suppose some malicious site may come out that splits things such that the first digit goes in the first, two in the next, and the remaining in a third... That may just mean fields that lack pre-defined division rules need some way to express them.
This can be a schema improvement for later, though.
There was a problem hiding this comment.
Yeah, I was also thinking about SSN entry forms and partial entry forms like SSN and credit card confirmation and totp-like separated entries for phone numbers. I played with trying to abstract that split more definitively, but ended up with some unwieldy patterns. I think I'm leaning toward the opinion that grouped partial information deserves it's own descriptor when it's not a uniform distribution, but I agree we can come back to it when we've had more time using the map.
some malicious site may come out
If we're curating a map for the site, it means we can't represent something on the page (that we'd otherwise want to), at worst. If the site/page doing something actively malicious, we have other mechanisms to communicate/defend against that.
differsthecat
left a comment
There was a problem hiding this comment.
This is awesome, thank you, @jprusik ! 🚀
| There is presently no mechanism embedded within the Forms Map for: | ||
|
|
||
| - describing the age of individual host entries | ||
| - annotating entries (descriptions of why a particular selector is needed, etc.) |
There was a problem hiding this comment.
Non-blocking: The file format for forms is jsonc, which should allow us to add comments, so we could annotate an entry with comments (https://jsonc.org/), unless that messes with the validation of the entries.
|
Great job! No new security vulnerabilities introduced in this pull request |
|
Merged as discussed with @MGibson1 - we will continue to refine and will have at least one final review before public release |

🎟️ Tracking
PM-33948
PM-33949
📔 Objective
This PR primarily pulls in the working state from the draft project at https://github.com/jprusik/map-the-web-draft (you can also get a sense for how the releases look there)
The project provides curated JSON "Maps" that describe websites. These changes represent the form Map which describes form field locations using CSS selectors, enabling consumers (like the Bitwarden browser extension) to locate fields without relying solely on heuristics.
The READMEs cover most of the guidance (which represents the most important part of this review), but I'll add a few notes about some decisions made along the way:
>>>combinator in CSS selectors to describe iframe and shadow dom boundaries. There are some examples of prior art: WebDriverIO | Puppeteer | originated in (abandoned) w3 specthe key names for selectors are still a bit in flux (as called out in the README and schema); I opted to come back to them to fight scope creep (see PM-34086)edit: I've addressed this in 93e3152 to get us started with something more concretewww.prefixed hosts technically can behave differently than their non-www counterpart. This scenario is described in the guidance (only includewwwin the host key name when needed), but the checks only warn about this scenario right now. It's edgy enough that I don't think we need to dive deeply into it, but if we run into these cases commonly, we might re-visit the tooling.