430910 - Rich text editor: pasted links have wrong number of leading "../" if editor page query string contains "/"

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Cacycle]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.14) Gecko/20080404 Firefox/2.0.0.14

Links pasted into an iframe in designmode have the wrong number of leading "../" if page of the iframe page contains "/" as part of the query string.

I guess that the query string should theoretically not contain a "/" because it is supposed to be URLencoded. However, major websites including Wikipedia make use of such links. It should be relatively easy to fix this by stopping to parse the URL at the first "?". 

This bug currently breaks the HTML-to-wikicode conversion of the Wikipedia editor wikEd.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.kevinroth.com/rte/demo.htm?test=a/b/c/d/e/f/g/h
2. Paste the link above the edit window into the edit area
3. Check the "Show source" box 
Actual Results:  
Page URL: http://www.kevinroth.com/rte/demo.htm?test=a/b/c/d/e/f/g/h
Link URL: http://www.kevinroth.com/rte/
Pasted HTML: <a href="../../../../../../../">Cross-Browser Rich Text Editor (RTE) home page</a>

Expected Results:  
Correct HTML: <a href="./">Cross-Browser Rich Text Editor (RTE) home page</a>

Comment 1

[Cacycle]

Still broken in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1 (.NET CLR 3.5.30729). This bug still affects web applications, including the Wikipedia editor wikEd and it is not possible to create a workaround. Please also note the high number of votes. This might be a very simple bug to fix.

Comment 2

[Cacycle]

Still broken in 3.7a1pre. Please, can somebody fix this. It looks like a really easy to fix bug and it breaks a major feature of the Wikipedia editor wikEd (since almost two years!).

(Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090927 Minefield/3.7a1pre (.NET CLR 3.5.30729))

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: stack showing where we hit the buggy code

I was thinking this would be a case of editor doing its own URL mangling.  However, it's not; this is a bug in nsStandardURL::GetRelativeSpec:

(gdb) p *this
$44 = (nsStandardURL) {<nsIFileURL> = {<nsIURL> = {<nsIURI> = {<nsISupports> = {_vptr.nsISupports = 0x7f4b07440330}, <No data fields>}, <No data fields>}, <No data fields>}, <nsIStandardURL> = {<nsIMutable> = {<nsISupports> = {
        _vptr.nsISupports = 0x7f4b07440550}, <No data fields>}, <No data fields>}, <nsISerializable> = {<nsISupports> = {
      _vptr.nsISupports = 0x7f4b07440590}, <No data fields>}, <nsIClassInfo> = {<nsISupports> = {_vptr.nsISupports = 0x7f4b074405c8}, <No data fields>}, 
  mRefCnt = {mValue = 13}, _mOwningThread = {mThread = 0x144aab0}, 
  mSpec = {<nsACString_internal> = {
      mData = 0x43c2718 "http://www.kevinroth.com/rte/demo.htm?test=a/b/c/d/e/f/g/h", mLength = 58, mFlags = 5}, <No data fields>}, mDefaultPort = 80, 
  mPort = -1, mScheme = {mPos = 0, mLen = 4}, mAuthority = {mPos = 7, 
    mLen = 17}, mUsername = {mPos = 7, mLen = -1}, mPassword = {mPos = 7, 
    mLen = -1}, mHost = {mPos = 7, mLen = 17}, mPath = {mPos = 24, mLen = 34}, 
  mFilepath = {mPos = 24, mLen = 13}, mDirectory = {mPos = 24, mLen = 5}, 
  mBasename = {mPos = 29, mLen = 4}, mExtension = {mPos = 34, mLen = 3}, 
  mParam = {mPos = 24, mLen = -1}, mQuery = {mPos = 38, mLen = 20}, mRef = {
    mPos = 24, mLen = -1}, mOriginCharset = {<nsACString_internal> = {
      mData = 0x7f4b16f1cb68 "", mLength = 0, mFlags = 1}, <No data fields>}, 
  mParser = {mRawPtr = 0x1662270}, mFile = {mRawPtr = 0x0}, mHostA = 0x0, 
  mHostEncoding = 1, mSpecEncoding = 1, mURLType = 2, mMutable = 0, 
  mSupportsFileURL = 0, static gIDN = 0x1632100, static gCharsetMgr = 0x0, 
  static gInitialized = 1, static gEscapeUTF8 = 1, 
  static gAlwaysEncodeInUTF8 = 1, static gEncodeQueryInUTF8 = 0, 
  mDebugCList = {next = 0x2e1dd90, prev = 0x2c5a610}}
(gdb) p *uri2
$45 = (nsStandardURL) {<nsIFileURL> = {<nsIURL> = {<nsIURI> = {<nsISupports> = {_vptr.nsISupports = 0x7f4b07440330}, <No data fields>}, <No data fields>}, <No data fields>}, <nsIStandardURL> = {<nsIMutable> = {<nsISupports> = {
        _vptr.nsISupports = 0x7f4b07440550}, <No data fields>}, <No data fields>}, <nsISerializable> = {<nsISupports> = {
      _vptr.nsISupports = 0x7f4b07440590}, <No data fields>}, <nsIClassInfo> = {<nsISupports> = {_vptr.nsISupports = 0x7f4b074405c8}, <No data fields>}, 
  mRefCnt = {mValue = 1}, _mOwningThread = {mThread = 0x144aab0}, 
  mSpec = {<nsACString_internal> = {
      mData = 0x274dc78 "http://www.kevinroth.com/rte/", mLength = 29, 
      mFlags = 5}, <No data fields>}, mDefaultPort = 80, mPort = -1, 
  mScheme = {mPos = 0, mLen = 4}, mAuthority = {mPos = 7, mLen = 17}, 
  mUsername = {mPos = 7, mLen = -1}, mPassword = {mPos = 7, mLen = -1}, 
  mHost = {mPos = 7, mLen = 17}, mPath = {mPos = 24, mLen = 5}, mFilepath = {
    mPos = 24, mLen = 5}, mDirectory = {mPos = 24, mLen = 5}, mBasename = {
    mPos = 29, mLen = 0}, mExtension = {mPos = 29, mLen = -1}, mParam = {
    mPos = 24, mLen = -1}, mQuery = {mPos = 24, mLen = -1}, mRef = {mPos = 24, 
    mLen = -1}, mOriginCharset = {<nsACString_internal> = {
      mData = 0x7f4b16f1cb68 "", mLength = 0, mFlags = 1}, <No data fields>}, 
  mParser = {mRawPtr = 0x1662270}, mFile = {mRawPtr = 0x0}, mHostA = 0x0, 
  mHostEncoding = 1, mSpecEncoding = 0, mURLType = 2, mMutable = 1, 
  mSupportsFileURL = 0, static gIDN = 0x1632100, static gCharsetMgr = 0x0, 
  static gInitialized = 1, static gEscapeUTF8 = 1, 
  static gAlwaysEncodeInUTF8 = 1, static gEncodeQueryInUTF8 = 0, 
  mDebugCList = {next = 0x7f4b0744c680, prev = 0x3efe510}}
(gdb) p aResult
$46 = (nsACString_internal &) @0x7fff2438c860: {
  mData = 0x7fff2438c880 "../../../../../../../", mLength = 21, mFlags = 65553}

Comment 4

[Boris Zbarsky [:bzbarsky]]

1981     // need to account for slashes and add corresponding "../"
1982     while (*thisIndex)
1983     {
1984         if (*thisIndex == '/')
1985             aResult.AppendLiteral("../");
1986 
1987         thisIndex++;
1988     }

We should not be advancing past mSpec.get() + mParam.mPos or so (some of those positions might be -1, so have to be a little careful).

Comment 5

[Michael]

This bug still affects Gecko-20100818(Firefox 4.0b4). In Safari 5.01 the pasted url is <a href="http://www.kevinroth.com/rte/">Cross-Browser Rich Text Editor (RTE) home page</a>.
There's no attempt to convert the url to relative. I would like to look into resolving this.

Comment 6

[Michael]

I think I have a patch for Gecko/20100902 - Minefield/4.0b4. Should I upload it.

Comment 7

[(no longer active)]

(In reply to comment #6)
> I think I have a patch for Gecko/20100902 - Minefield/4.0b4. Should I upload
> it.

Please!

Comment 8

[Michael]

Attachment: patch for nsStandardURL::GetRelativeSpec

This patch was made against the 2.0b4 source. This should fix the problem described here. I also noticed this comment.
// todo:  also check for file matches with '#', '?' and ';'
Should this be a separate issue?

Comment 9

[Michael]

> Should this be a separate issue?
Never-mind I'm confusing myself.

Comment 10

[Michael]

Attachment: revised b5 patch

Comment 11

[Michael]

still b4 source for compare.

Comment 12

[Michael Kohler [:mkohler]]

Comment on attachment 475864 [details] [diff] [review]
revised b5 patch

I have no idea about this bug/code/module and I'm not a peer anyway -> canceling review.

Comment 13

[(no longer active)]

Attachment: Patch (v1)

I think this is the correct fix here...

Comment 14

[Michael]

Comment on attachment 478340 [details] [diff] [review]
Patch (v1)

Looks much better.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 478340 [details] [diff] [review]
Patch (v1)

Was there a reason that:

    const char *limit = mSpec.get() + mFilePath.mPos + mFilePath.mLen;

(without any subtraction) didn't work?

Comment 16

[(no longer active)]

Attachment: Patch (v2)

(In reply to comment #15)
> Comment on attachment 478340 [details] [diff] [review]
> Patch (v1)
> 
> Was there a reason that:
> 
>     const char *limit = mSpec.get() + mFilePath.mPos + mFilePath.mLen;
> 
> (without any subtraction) didn't work?

I must have missed it somehow, but it definitely does work.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 478838 [details] [diff] [review]
Patch (v2)

r=me

I'd like someone else to look at the approval request, though... I do think this is safe enough.

Comment 18

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/4f09e912977d