Backslashes included in a location bar input are converted to forward slashes on submission

VERIFIED FIXED in Firefox 33

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: avaida, Assigned: alexbardas)

Tracking

({regression})

34 Branch
mozilla34
All
Windows 7
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox31 unaffected, firefox32 unaffected, firefox33+ verified, firefox34+ verified, firefox35 verified)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Reporter

Description

5 years ago
Note: this is a follow-up for Bug 693808.

Reproducible on Nightly 34.0a1 (2014-07-22), Build ID: 20140722030201, Windows platform only.

Steps to reproduce:
1. Launch Nightly 34.
2. Bring the Location Bar into focus and type in 'test\' without the apostrophe.
3. Press the <enter> key.

Expected result: 
A google search is performed for the 'test\' keyword (w/o apostrophe).

Actual result: 
A google search is performed for the 'test/' keyword (w/o apostrophe), with the backslash converted to a forward slash.
Flags: firefox-backlog+

Comment 1

5 years ago
Alex, can I interest you in this bug? :-)
Flags: needinfo?(abardas)
Assignee

Comment 2

5 years ago
(In reply to :Gijs Kruitbosch from comment #1)
> Alex, can I interest you in this bug? :-)

Yes, I will look into it :).

I guess we should have a regression test with a comment that it's only for windows?
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Flags: needinfo?(abardas)

Comment 3

5 years ago
(In reply to Alex Bardas :alexbardas from comment #2)
> I guess we should have a regression test with a comment that it's only for
> windows?

You can add a case in the Windows-only if block in the existing test for nsIURIFixup that you've been adding to in the lat/long and ipv6 bug. :-)
Hi Alex, can you assign a point value and if the bug should be marked as qe-verify+ or qe-verify- for verification.  Thanks.
Iteration: --- → 34.3
Flags: qe-verify?
Flags: needinfo?(abardas)
Assignee

Updated

5 years ago
Points: --- → 1
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(abardas)
Reporter

Updated

5 years ago
QA Contact: andrei.vaida

Comment 6

5 years ago
Comment on attachment 8477642 [details] [diff] [review]
Limit cases when backslashes are converted to slashes for windows machines + regression test

Review of attachment 8477642 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, no, I think you should just ensure that if we hit the keywordtouri path, we use the original input. Because we store that in the nsIURIFixupInfo object, we can just use that instead of the url-mangled version of the input.
Attachment #8477642 - Flags: review?(gijskruitbosch+bugs) → feedback-
Assignee

Comment 7

5 years ago
I also added 2 tests, 1 for windows and 1 for os x + linux.
Attachment #8477642 - Attachment is obsolete: true
Attachment #8478566 - Flags: review?(gijskruitbosch+bugs)

Comment 8

5 years ago
Comment on attachment 8478566 [details] [diff] [review]
Limit cases when backslashes are converted to slashes for windows machines + regression tests

Review of attachment 8478566 [details] [diff] [review]:
-----------------------------------------------------------------

This is what I meant, yeah. As discussed on IRC, can you add a test for the case that includes spaces to ensure we're trimming the input that goes to the search engine?
Attachment #8478566 - Flags: review?(gijskruitbosch+bugs) → feedback+
Comment on attachment 8478611 [details] [diff] [review]
Use the original input url instead of the modified one for KeywordURIFixup + regression tests

r=me.  Thanks for the patch!
Attachment #8478611 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/88a47c075155
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34

Comment 13

5 years ago
Alex, could you request approval for aurora? :-)
Flags: needinfo?(abardas)

Updated

5 years ago
Keywords: regression
Whiteboard: [fixed-in-inbound]
Comment on attachment 8478611 [details] [diff] [review]
Use the original input url instead of the modified one for KeywordURIFixup + regression tests

Review of attachment 8478611 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/test/unit/test_nsDefaultURIFixup_info.js
@@ +76,5 @@
>  
>  function run_test() {
>    for (let [testInput, expectedFixedURI, alternativeURI,
>              expectKeywordLookup, expectProtocolChange] of testcases) {
> +    testInput = testInput.trim();

(In reply to :Gijs Kruitbosch from comment #8)
> can you add a test for the
> case that includes spaces to ensure we're trimming the input that goes to
> the search engine?

Er, because the test trims the input before it gets passed to urifixup, it's not actually testing this.
Assignee

Comment 15

5 years ago
The initial input must be trimmed when it is compared with the result, not before calling urifixup.

Blair, if this patch fixes the problem, how can it be pushed before requesting approval for aurora?
Attachment #8481055 - Flags: review?(bmcbride)
Reporter

Comment 16

5 years ago
This specific test case is verified fixed on Nightly 34.0a1 (2014-08-28) using Windows 7 64-bit, but it seems that 'test \' (containing a space as well) is still converted to 'test /'. Also re-confirmed that this does *not* affect Linux and Mac.

Waiting for a new patch for this bug, per my conversation with Alex.
Assignee

Comment 17

5 years ago
(In reply to Andrei Vaida, QA [:avaida] from comment #16)
> This specific test case is verified fixed on Nightly 34.0a1 (2014-08-28)
> using Windows 7 64-bit, but it seems that 'test \' (containing a space as
> well) is still converted to 'test /'. Also re-confirmed that this does *not*
> affect Linux and Mac.
> 
> Waiting for a new patch for this bug, per my conversation with Alex.

So I further investigated this. In case there is a " " in the url, then that condition from KeywordURIFixup is not reached anymore, because there is a space, no dots, no colons, no quotes, no question marks and the first condition is reached. I guess we also need to use the originalInput there. I'll post a patch addressing that and waiting for feedback.

In this case
Assignee

Comment 18

5 years ago
Attachment #8481055 - Attachment is obsolete: true
Attachment #8481055 - Flags: review?(bmcbride)
Attachment #8481125 - Flags: review?(gijskruitbosch+bugs)
Attachment #8481125 - Flags: review?(bmcbride)

Comment 19

5 years ago
Comment on attachment 8481125 [details] [diff] [review]
Do not trim the input before calling urifixup

Review of attachment 8481125 [details] [diff] [review]:
-----------------------------------------------------------------

While we're here, let's also fix: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDefaultURIFixup.cpp#396

This part is broken on release as well. Test with something like: "mozilla \ foo.txt" as input.
Attachment #8481125 - Flags: review?(gijskruitbosch+bugs)
Attachment #8481125 - Flags: review?(bmcbride)
Attachment #8481125 - Flags: feedback+

Comment 20

5 years ago
(also, can we add a test for the \r\n stripping, too? :-) )

Comment 21

5 years ago
(In reply to :Gijs Kruitbosch from comment #19)
> Comment on attachment 8481125 [details] [diff] [review]
> Do not trim the input before calling urifixup
> 
> Review of attachment 8481125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> While we're here, let's also fix:
> http://mxr.mozilla.org/mozilla-central/source/docshell/base/
> nsDefaultURIFixup.cpp#396

Err, wait. I misread, I think - this is probably fine? Maybe?

> This part is broken on release as well. Test with something like: "mozilla \
> foo.txt" as input.

This is still true, but is probably still the same path as comment #16, which is also broken on release, so the existing patch probably fixes this?

I'm not sure if the comment above the earlier link is wrong, and/or if that call should be updated anyway, because right now it's passing the untrimmed string, which seems like it'd be wrong. Just not sure offhand what cases hit that path, I'd need to chuck this through a debugger. One more reason that all of this should just move to JS... :-\
Assignee

Comment 22

5 years ago
I've added more tests and tested on OS X & Windows. 

I've also reversed the order in which we modify the input in the docshell file. First we need to strip "\r|\n" and then to trim, otherwise "file \r\n" would become "file ". KeywordToURI does another trim though.

I can't think of an use case when we would need the trim before, I hope this change is appropriate.
Attachment #8481125 - Attachment is obsolete: true
Attachment #8481438 - Flags: review?(gijskruitbosch+bugs)
Flags: needinfo?(abardas)

Comment 23

5 years ago
Comment on attachment 8481438 [details] [diff] [review]
Do not trim the input before calling urifixup

Review of attachment 8481438 [details] [diff] [review]:
-----------------------------------------------------------------

This LGTM, but you need a docshell peer's review :-)

::: docshell/test/unit/test_nsDefaultURIFixup_info.js
@@ +82,5 @@
>    testcases.push(["mozilla\\", "http://mozilla\\/", "http://www.mozilla/", true, true]);
>  }
>  
> +function sanitize(input) {
> +  return input.replace(/\r|\n|\r\n/g, "").trim();

Nit: don't need the third 'or'd bit, ie just /\r|\n/g should do
Attachment #8481438 - Flags: review?(gijskruitbosch+bugs)
Attachment #8481438 - Flags: review?(bzbarsky)
Attachment #8481438 - Flags: feedback+
Assignee

Comment 24

5 years ago
(In reply to :Gijs Kruitbosch from comment #23)
> Comment on attachment 8481438 [details] [diff] [review]
> Do not trim the input before calling urifixup
> 
> Review of attachment 8481438 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: docshell/test/unit/test_nsDefaultURIFixup_info.js
> @@ +82,5 @@
> >  
> > +function sanitize(input) {
> > +  return input.replace(/\r|\n|\r\n/g, "").trim();
> 
> Nit: don't need the third 'or'd bit, ie just /\r|\n/g should do

I realized that but forgot to hg qref it in the last patch. I will modify it after bz's review :-).

I'm also changing the bug status to incomplete.
Resolution: FIXED → INCOMPLETE

Comment 25

5 years ago
Incomplete tends to mean "the report didn't contain enough information for us to address it in any way". That's not the case here - this is just not fixed yet, so let's reopen it. :-)
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
Comment on attachment 8481438 [details] [diff] [review]
Do not trim the input before calling urifixup

r=me
Attachment #8481438 - Flags: review?(bzbarsky) → review+
Assignee

Updated

5 years ago
Attachment #8481467 - Flags: checkin+
Assignee

Comment 28

5 years ago
The second patch needs checkin. The first one was already applied.
Keywords: checkin-needed

Updated

5 years ago
Attachment #8478611 - Flags: checkin+

Updated

5 years ago
Attachment #8481467 - Flags: checkin+ → checkin?
Iteration: 34.3 → 35.1
Attachment #8481467 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/fd07f405c3a0
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Reporter

Comment 31

5 years ago
Both scenarios are now verified fixed on Nightly 35.0a1 (2014-09-02) using Windows 7 64-bit.

Acceptance criteria for this patch:
- Backslashes (e.g. 'test\') are not converted to forward slashes.
- Backslashes preceded by a space (e.g. 'test \') are not converted to forward slashes.
Status: RESOLVED → VERIFIED
Reporter

Comment 32

5 years ago
Alex, shouldn't we flip the status flag for Firefox 34 back to affected, since the 2nd patch (that verifies this bug completely) is only available on Nightly 35? I'm thinking that, this way, it would be less confusing.
Flags: needinfo?(abardas)

Comment 33

5 years ago
(In reply to Andrei Vaida, QA [:avaida] from comment #32)
> Alex, shouldn't we flip the status flag for Firefox 34 back to affected,
> since the 2nd patch (that verifies this bug completely) is only available on
> Nightly 35? I'm thinking that, this way, it would be less confusing.

Yes. Alex, can you request aurora approval for the second patch, and add a combined patch and request beta approval on that? We should ideally uplift all of this to beta to land together with the other search-in-the-urlbar fixes we did.
Assignee

Comment 34

5 years ago
Comment on attachment 8481467 [details] [diff] [review]
Better test coverage + do not trim the input before calling urifixup in tests

Approval Request Comment
[Feature/regressing bug #]: bug 693808
[User impact if declined]: affects windows users which use "\" in keyword searches
[Describe test coverage new/current, TBPL]: has automated tests + was manually tested
[Risks and why]: small change of the behavior of the location bar
[String/UUID change made/needed]: no
Attachment #8481467 - Flags: approval-mozilla-aurora?
Flags: needinfo?(abardas)
Comment on attachment 8481467 [details] [diff] [review]
Better test coverage + do not trim the input before calling urifixup in tests

Aurora+

33 (Beta) is marked as affected as well. Can you please request beta uplift as well so that we can avoid shipping this regression?
Attachment #8481467 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(abardas)
Reporter

Comment 37

5 years ago
Verified fixed on Aurora 34.0a2 (2014-09-17) as well, using Windows 7 64-bit.
Comment on attachment 8481467 [details] [diff] [review]
Better test coverage + do not trim the input before calling urifixup in tests

[Triage Comment]
bz thinks it is safe to take it in beta too. Approving it now to have it in beta 6.
Attachment #8481467 - Flags: approval-mozilla-beta+
Flags: needinfo?(abardas)
Reporter

Comment 40

5 years ago
Verified fixed on Firefox 33.0b6 (20140922173023 / d508b53c3dee) using Windows 7 64-bit.
You need to log in before you can comment on or make changes to this bug.