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.

Sign up for GitHub

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

Jump to bottom

Add FTP related protocols to the registerProtocolHandler safelist. #6584

Merged
merged 1 commit into from
Feb 14, 2022
Merged

Add FTP related protocols to the registerProtocolHandler safelist. #6584

merged 1 commit into from
Feb 14, 2022

Conversation

asankah
Copy link
Contributor

@asankah asankah commented Apr 14, 2021 •
edited by pr-preview bot
Loading

Closes #6583

(See WHATWG Working Mode: Changes for more details.)

/system-state.html ( diff )

/acknowledgements.html ( diff )
/system-state.html ( diff )

@asankah asankah mentioned this pull request Apr 15, 2021
Closed
@annevk annevk mentioned this pull request Apr 28, 2021
Closed
3 tasks
@asankah
Copy link
Contributor Author

asankah commented May 21, 2021

Adding @domenic as well.

I updated the PR description to reflect that Firefox has reacted positively to the proposed change.

@annevk
Copy link
Member

annevk commented May 25, 2021 •
edited
Loading

In the discussion with @valenting you mentioned removing credentials. Is that something you ended up pursuing? If so, it should probably be part of the standard, no? (Although I suppose we can only really do it for ftp, I'm not sure if that makes sense then.)

@annevk annevk added addition/proposal New features or enhancements topic: custom protocols labels May 25, 2021
@domenic
Copy link
Member

domenic commented May 26, 2021

Yes, per the change in https://chromium-review.googlesource.com/c/chromium/src/+/2826871 it looks like that is what was implemented. @asankah can you update this PR to specify that behavior?

@aaronraimist aaronraimist mentioned this pull request Jul 31, 2021
Merged
@domenic
Copy link
Member

domenic commented Aug 17, 2021

@asankah ping on updating the PR to match what Chromium does.

@asankah
Copy link
Contributor Author

asankah commented Oct 9, 2021

@domenic @annevk I looked at adding it, but the spec already contains the following text:

Leaking credentials. User agents must never send username or password information in the URLs that are escaped and included sent to the handler sites.

This already captures what was agreed upon regarding how ftp/ftps/sftp should be handled.

@annevk
Copy link
Member

annevk commented Oct 11, 2021

@asankah the ask is to make that explicit in the processing model. And perhaps it should be exclusively in the processing model, as it's not clear if that note also applies to mailto:[email protected] for instance (it shouldn't, because that would make the feature useless).

@asankah
Copy link
Contributor Author

asankah commented Oct 11, 2021

@annevk Ohh. Gotcha. I'll update the patch to address that.

@annevk
Copy link
Member

annevk commented Jan 21, 2022

Hey @asankah, is this still on your radar? We've been thinking of shipping this behavior in Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1705202.

cc @valenting

@hamishwillee
Copy link

hamishwillee commented Feb 7, 2022 •
edited
Loading

@asankah @annevk Any more news on this? I'm hoping to update the MDN documentation, but if https://bugzilla.mozilla.org/show_bug.cgi?id=1705202 goes in and this PR does not then I will need to update browser compatibility data in a more complicated way, as this will end up being non-spec behavior in a released platform

@hamishwillee hamishwillee mentioned this pull request Feb 7, 2022
Closed
7 tasks
@asankah
Copy link
Contributor Author

asankah commented Feb 7, 2022

Whoops. Yeah, on it.

@domenic
Copy link
Member

domenic commented Feb 7, 2022

The rebase appears to have gone poorly, could you retry?

domenic
domenic reviewed Feb 7, 2022
View reviewed changes
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@asankah asankah force-pushed the main branch 2 times, most recently from 223c6bb to 18694ba Compare February 7, 2022 18:24
asankah
asankah commented Feb 7, 2022
View reviewed changes
source
@@ -98495,14 +98512,6 @@ interface <dfn interface>Navigator</dfn> {
allowing administrators to disable custom handlers on certain subdomains, content types, or
schemes.</p>

<p><strong>Leaking credentials.</strong> User agents must never send username or password
Copy link
Contributor Author

@asankah asankah Feb 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I removed the Leaking Credentials section since it is addressed in the processing steps.

The part about resources that may require credentials is, I believe, addressed in the "Leaking private data" section which warns about private URLs in general.

domenic
domenic approved these changes Feb 7, 2022
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Will give it a day in case @annevk wants to do an additional review.

asankah added a commit to asankah/web-platform-tests that referenced this pull request Feb 7, 2022
@asankah
Add registerProtocolHandler tests for FTP.
d4901db 
`registerProtocolHandler` allows registering `ftp`, `ftps`, and `sftp`
protocols as of whatwg/html#6584.

This change adds tests to ensure that `registerProtocolHandler` allows
registering those protocols and to ensure that embedded credentials are
removed prior to invoking the custom protocol handler.
asankah added a commit to asankah/web-platform-tests that referenced this pull request Feb 7, 2022
@asankah
Add registerProtocolHandler tests for FTP.
befefdb 
`registerProtocolHandler` allows registering `ftp`, `ftps`, and `sftp`
protocols as of whatwg/html#6584.

This change adds tests to ensure that `registerProtocolHandler` allows
registering those protocols and to ensure that embedded credentials are
removed prior to invoking the custom protocol handler.
asankah added a commit to asankah/web-platform-tests that referenced this pull request Feb 7, 2022
@asankah
Add registerProtocolHandler tests for FTP.
434cda0 
`registerProtocolHandler` allows registering `ftp`, `ftps`, and `sftp`
protocols as of whatwg/html#6584.

This change adds tests to ensure that `registerProtocolHandler` allows
registering those protocols and to ensure that embedded credentials are
removed prior to invoking the custom protocol handler.
asankah added a commit to asankah/web-platform-tests that referenced this pull request Feb 7, 2022
@asankah
Add registerProtocolHandler tests for FTP.
eed042f 
`registerProtocolHandler` allows registering `ftp`, `ftps`, and `sftp`
protocols as of whatwg/html#6584.

This change adds tests to ensure that `registerProtocolHandler` allows
registering those protocols and to ensure that embedded credentials are
removed prior to invoking the custom protocol handler.
asankah added a commit to asankah/web-platform-tests that referenced this pull request Feb 8, 2022
@asankah
Add registerProtocolHandler tests for FTP.
88c5627 
`registerProtocolHandler` allows registering `ftp`, `ftps`, and `sftp`
protocols as of whatwg/html#6584.

This change adds tests to ensure that `registerProtocolHandler` allows
registering those protocols and to ensure that embedded credentials are
removed prior to invoking the custom protocol handler.
asankah added a commit to asankah/web-platform-tests that referenced this pull request Feb 8, 2022
@asankah
Add registerProtocolHandler tests for FTP.
2124091 
`registerProtocolHandler` allows registering `ftp`, `ftps`, and `sftp`
protocols as of whatwg/html#6584.

This change adds tests to ensure that `registerProtocolHandler` allows
registering those protocols and to ensure that embedded credentials are
removed prior to invoking the custom protocol handler.
@asankah asankah mentioned this pull request Feb 8, 2022
Open
@hamishwillee
Copy link

@domenic No comment from @annevk - should this go in?

@domenic domenic merged commit 5b01d31 into whatwg:main Feb 14, 2022
@annevk annevk mentioned this pull request Dec 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domenic domenic domenic approved these changes

Assignees
No one assigned
Labels
addition/proposal New features or enhancements topic: custom protocols
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add FTP related protocols to the registerProtocolHandler safelist.
4 participants
@asankah @annevk @domenic @hamishwillee