Closed Bug 1624820 Opened 5 years ago Closed 5 years ago

aboutNetError's setNetErrorMessageFromCode likes to pass empty strings to document.l10n.formatValues which produces errors

Categories

(Firefox :: Security, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 77
Tracking Status
firefox-esr68 --- unaffected
firefox74 --- wontfix
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 --- verified

People

(Reporter: Gijs, Assigned: aarushivij)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

We apparently don't hit this in automation or it'd cause failures there, I'm pretty sure.

STR:

  1. open nightly with a new profile
  2. open youtube

ER:

No internal error messages

AR:

console.warn: "[fluent] Missing translations in en-US: ."

This is because we're falling back to an empty error code string, which we then pass to formatValue.

We shouldn't do this - we should avoid calling formatValue if we don't have a string to look for. Also, there should be a test for whatever is happening behind the scenes here.

Johann, is there a good component for this stuff?

Flags: needinfo?(jhofmann)

The neterror URI triggering this is:

about:neterror?e=xfoBlocked&u=https%3A//accounts.google.com/ServiceLogin%3Fuilel%3D3%26service%3Dyoutube%26continue%3Dhttps%253A%252F%252Fwww.youtube.com%252Fsignin%253Fhl%253Den%2526feature%253Dpassive%2526action_handle_signin%253Dtrue%2526next%253D%25252Fsignin_passive%2526app%253Ddesktop%26hl%3Den%26passive%3Dtrue&s=neterror&c=UTF-8&f=regular&d=This%20page%20has%20an%20X-Frame-Options%20policy%20that%20prevents%20it%20from%20being%20loaded%20in%20this%20context.

For a subframe with URL

https://accounts.google.com/ServiceLogin?uilel=3&service=youtube&continue=https%3A%2F%2Fwww.youtube.com%2Fsignin%3Fhl%3Den%26feature%3Dpassive%26action_handle_signin%3Dtrue%26next%3D%252Fsignin_passive%26app%3Ddesktop&hl=en&passive=true

Also, there should be a test for whatever is happening behind the scenes here.

You probably know but what happens is that a website gets embedded by Youtube (accounts.google.com) which sets an XFO header that results in a neterror. Not sure how or why we would test that, really?

aboutNetError.js then tries to get an error string for displaying an error message here and consults document.getNetErrorInfo which tries to get it from the transportSecurityInfo object here. Since there's no error string it falls back to an empty string.

To avoid the fluent warning we should just handle that "no error code string" scenario a bit better. There's lots of potential for refactoring here once we get to full fluent, I guess. But for now a small refactoring to return early in aboutNetError.js if errorCodeStr is empty instead of trying to set a translation for "empty string" would likely be enough.

Flags: needinfo?(jhofmann)
Component: General → Security
Priority: -- → P3

(In reply to Johann Hofmann [:johannh] from comment #2)

Also, there should be a test for whatever is happening behind the scenes here.

You probably know but what happens is that a website gets embedded by Youtube (accounts.google.com) which sets an XFO header that results in a neterror. Not sure how or why we would test that, really?

I think we could fairly easily do the same from a test? Send an XFO header, that is... Why wouldn't be able to do that?

aboutNetError.js then tries to get an error string for displaying an error message here and consults document.getNetErrorInfo which tries to get it from the transportSecurityInfo object here. Since there's no error string it falls back to an empty string.

To avoid the fluent warning we should just handle that "no error code string" scenario a bit better. There's lots of potential for refactoring here once we get to full fluent, I guess. But for now a small refactoring to return early in aboutNetError.js if errorCodeStr is empty instead of trying to set a translation for "empty string" would likely be enough.

Right.

I think we could fairly easily do the same from a test? Send an XFO header, that is... Why wouldn't be able to do that?

Right, from a quick look at the bug I assumed that you thought the fact that the error page itself is loaded was a problem, so I meant "how would we test errors in Google's internal account scheme". But reading it again that assumption probably wasn't true. :)

I guess this would throw in automation, right? So it seems that we don't have tests for showing neither the XFO nor CSP error pages? Christoph, do you know if these tests exist? If they do, it might be interesting to figure out why the uncaught error isn't detected in automation.:

Anyway, probably doesn't have any direct implications on the fix of this bug, so I'll see if I can find someone to fix it :)

Flags: needinfo?(ckerschb)

(In reply to Johann Hofmann [:johannh] from comment #4)

I guess this would throw in automation, right? So it seems that we don't have tests for showing neither the XFO nor CSP error pages? Christoph, do you know if these tests exist? If they do, it might be interesting to figure out why the uncaught error isn't detected in automation.:

There are no such tests but obviously we should have such, not only for x-frame-options but also for CSP frame-ancestors.

FWIW, I filed Bug 1626249 and assigned to myself.

Flags: needinfo?(ckerschb)

Hello, Can I take up this issue?
Thanks :)
Aarushi

Assignee: nobody → aarushivij
Status: NEW → ASSIGNED
Pushed by aiakab@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b40b0c09c1fe aboutNetError's setNetErrorMessageFromCode likes to pass empty strings to document.l10n.formatValues which produces errors r=johannh
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 77
  • Reproduced with Fx 76.0a1(2020-03-25) on Windows 10
  • Verified fixed with 78.0a1(2020-05-10) and 77.0b3 on Windows 10 and Ubuntu 18.04
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: