Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Field validations #11

Closed
guybedford opened this issue Mar 15, 2018 · 9 comments
Closed

Field validations #11

guybedford opened this issue Mar 15, 2018 · 9 comments

Comments

@guybedford
Copy link
Collaborator

What would be the exact form of the path, package name and scope name?

Enforcing all of these as bare names might roughly do the job, but does it cover it sufficiently?

Further, if I have one invalid package name in "packages", or one invalid scope name in "scopes", does this validate on startup? How does it throw / warn?

Do all other validations happen during resolution itself? If so, do their validations throw as the resolution error?

@domenic
Copy link
Collaborator

domenic commented Mar 15, 2018

I don't really understand this issue. What is a bare name? What is an invalid package name? What is an invalid scope name?

The current validation is specified in https://github.com/domenic/package-name-maps#the-package-name-map-format.

@guybedford
Copy link
Collaborator Author

I'm trying to think about what happens if I provide an absolute URL or full URL for a package name or scope name.

Bare names are as defined in the WhatWG loader spec resolver.

@guybedford
Copy link
Collaborator Author

Also dot segments in package names or scope names could cause trouble here as well.

@guybedford
Copy link
Collaborator Author

  • And trailing / segments in scope or package names are valid or removed?
  • And \ in scope or package names? Do these get converted to / like URL resolution or do they invalidate?

@domenic
Copy link
Collaborator

domenic commented Mar 15, 2018

Good point on package names. I guess absolute URLs will probably never be hit, since the "resolve a module specifier" algorithm will bail out before reaching that package. I'm not sure it's worth adding a linting step that prohibits these. Unsure about ..s...

Scopes are not names, but URLs, so absolute URLs or ..s are fine and useful there.

My current plan is to write the algorithm (#6) and a bunch of main-stream test cases, then see what behavior falls out for weird cases like this, and then reconsider this issue.

I don't think we have any plans to use the Loader draft here.

@guybedford
Copy link
Collaborator Author

Scopes are not names, but URLs, so absolute URLs or ..s are fine and useful there.

Thanks for clarifying this, all examples made this look like a package name, but URL makes a lot of sense - running through the various use cases again this seems to work out pretty well.

Scopes would need to be resolved to full URLs when the manifest is loaded in order to do scope detection, so scopes that fail URL resolution would potentially result in errors on manifest load.

I only mentioned bare names as the closest thing we have to what validations a package name goes through currently.

Great, I'd be happy to run edge cases against the draft when you have something written up.

@domenic
Copy link
Collaborator

domenic commented Jun 14, 2018

The up-front validation looks quite nice. Although, it makes it clear you're not validating the paths yet at all.

I still tend toward fewer validations on package names at least for now, although we should have a dedicated issue to discuss. But, disallowing forward slashes makes sense: forward slashes are special, as they are (by historical convention) what separates the package-name portion from the URL portion.

So I'd prefer:

  • Removing the existing package name validation except for the presence of /
  • Adding test cases showing that all the various bad things still work if used in a package name but mapped to a normal, boring path.
  • Adding test cases showing normal, boring package names mapped to scary paths.
  • Adding test cases showing scary package names implicitly mapped to scary paths by the default path logic.
  • Adding test cases for the / case, in both package names and paths.
  • Continue discussing how/whether we should tighten the restrictions in Field validations #11, once we have the test cases showing the weird things.

WDYT?

guybedford added a commit to guybedford/import-maps that referenced this issue Jun 17, 2018
guybedford added a commit to guybedford/import-maps that referenced this issue Jun 17, 2018
@guybedford
Copy link
Collaborator Author

I've updated the PR in #40 to only validate leading and trailing / in package names. This does seem like a sensible special case as you say.

I must admit I'm not as worried about path validation cases as it seems like the URL spec handles most of these cases for us. It could be interesting to work through in what scenarios will the URL spec will throw given a valid referrer as well.

domenic pushed a commit that referenced this issue Jun 26, 2018
These mostly follow from discussions in #11.

For now our validations only consist of:

- Type checks
- Package names must not start or end with /

Other cases like ".." or slashes elsewhere are currently passed through, with tests verifying that behavior. We'll discuss further restrictions in #11.
@domenic
Copy link
Collaborator

domenic commented Nov 2, 2018

Given that the proposal has been overhauled, let us merge this into #63, since this thread contains a lot of discussion of fields that no longer exist.

@domenic domenic closed this as completed Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants