1793560 - Remove (disabled by default) CSP directive `navigate-to`.

Status: RESOLVED FIXED

(Core :: DOM: Security, task, P4)

Description

[Frederik Braun [:freddy]]

The CSP directive navigate-to never shipped in Chrome or Firefox.
There are concerns about leaking redirect & cross-origin information and the editors suggest removing it from the specification

Let's remove our implementation too.

Comment 1

[Frederik Braun [:freddy]]

This can be tackled as a good first bug.
Requirements that must be met to take this bug:
(1) Some knowledge in C++
(2) a fully working environment that allows you to modify and build Firefox.

For (2), please work through the recommended steps at https://wiki.mozilla.org/Good_first_bug and https://firefox-source-docs.mozilla.org/setup/contributing_code.html.

If you encounter any road blocks in setting up your development environment, please consult a search engine or our #introduction chat on Matrix. - Not here.

Comment 2

[Oliver Medhurst [:canadahonk]]

Attachment: Bug 1793560 - Remove navigate-to CSP directive

It has never shipped in Firefox (or any browser) after being implemented
years ago, and was removed from spec in September 2022:
https://github.com/w3c/webappsec-csp/pull/564

TODO:

• Remove existing WPT tests and add historical not impl check?

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/117114b8eb32
Remove navigate-to CSP directive r=tschuster

Comment 4

[Cristina Horotan [:chorotan]]

Backed out changeset 117114b8eb32 (bug 1793560) for causing wpt failures at iframe-all-local-schemes-inherit-self.sub.html

Backout: https://hg.mozilla.org/integration/autoland/rev/18d91fb6569af8afe8d64c8cc5ec1b9508356501

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=117114b8eb321259fe5b9160d2501623b6848b7a&selectedTaskRun=GJoEQ__dQUiPlvMEpAmsVg.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=422137313&repo=autoland&lineNumber=2549

Comment 5

[Oliver Medhurst [:canadahonk]]

Not sure what's up with the failure specifically, changed to changes planned and will look into.

Comment 6

[Oliver Medhurst [:canadahonk]]

Not sure what the issue is after debugging locally for a while, but I can repro the fails of that WPT (content-security-policy/inheritance/iframe-all-local-schemes-inherit-self.sub.html). @tschuster do you mind taking a look if free at some point? Thanks!

Comment 7

[Tom Schuster]

Sorry CanadaHook, I won't be able to look at this before next week.

Comment 8

[Tom Schuster]

Okay, this turned out to be a much bigger rabbit hole than I initially expected. We already know that we have a bug with 'self' sources when the CSP is inherited to iframes . (We should be using the origin and not what we currently call mSelfURI, see bug 1721296 and bug 1803475)

However it looks like the call to GetAllowsNavigateTo in Document::StartDocumentLoad was partially working around this issue by accident! The first thing that GetAllowsNavigate does is to call EnsureIPCPoliciesRead, which will call AppendPolicy. AppendPolicy will then parse a CSP using the context's current mSelfURI, which at that point is still the previous mSelfURI (not about:srcdoc etc.).

When we remove this early call to GetAllowsNavigate we will instead end up calling AppendPolicy from Document::InitCSP (via GetCSPSandboxFlags specifically). This only happens after a call to nsCSPContext::SetRequestContextWithDocument, which changes the mSelfURI of the context to about:srcdoc and overrides whatever was in the CSP from loadInfo->GetCspToInherit().

We need to be really careful here not accidentally break web compatibility. I am not sure yet what the best way forward is.

Comment 9

[Oliver Medhurst [:canadahonk]]

Is it worth adding a hack to call EnsureIPCPoliciesRead/etc where the old GetAllowsNavigate was called for now or should we avoid it?

Comment 10

[Tom Schuster]

(In reply to Oliver Medhurst [:canadahonk] from comment #9)

Is it worth adding a hack to call EnsureIPCPoliciesRead/etc where the old GetAllowsNavigate was called for now or should we avoid it?

We probably should do something like this. A real solution for the 'self' problem isn't planned yet and we had some breakage when I accidentally broke it (more) before.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Comment 12

[Oliver Medhurst [:canadahonk]]

Happy to (try to) finish this off.

We probably should do something like this.

I'll update the patch with that hack and a reference to the problem, hopefully that makes it good to go.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f100b598ad7d
Remove navigate-to CSP directive r=tschuster

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f100b598ad7d