Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#15869 closed Bug (fixed)

CSRF fix for Ajax POST mentioned in docs intermittently fails to append token for IE7

Reported by: nick@… Owned by: nobody
Component: Documentation Version: 1.3
Severity: Normal Keywords: ajax csrf post
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description

I've found that in some cases(not sure why), IE7 will prepend protocol://servername to a form's action, causing the

 if (!(/^http:.*/.test(settings.url) || /^https:.*/.test(settings.url))) {...}

test to fail...

I propose we use the following instead, as it will work in more cases:

    var page_host = window.location.host;
    var regex=new RegExp('^https?://' + page_host + '/', 'i');
    if (regex.test(settings.url) || !(/^http:.*/.test(settings.url) || /^https:.*/.test(settings.url))) {
        // Only send the token to relative URLs i.e. locally.
    }

Attachments (0)

Change History (6)

comment:1 follow-up: Changed 3 years ago by lukeplant

  • Easy pickings unset
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Thanks. Can you check if this works when there is an explicit port number in the URL? (i.e. presumably IE will have to include this in the URL, but is it also included in window.location.host?)

comment:2 in reply to: ↑ 1 Changed 3 years ago by anonymous

Just checked, and yes, it does appear to work properly, even when a port is in the URL. Good idea, I hadn't thought of that!

Replying to lukeplant:

Thanks. Can you check if this works when there is an explicit port number in the URL? (i.e. presumably IE will have to include this in the URL, but is it also included in window.location.host?)

comment:3 Changed 3 years ago by lukeplant

Two problems:

1) the regex approach actually is incorrect if page_host contains any special characters like ., which it will do, opening up vulnerabilities. Escaping the special characters is one fix, but that is trickier than it ought to be, because javascript doesn't have built in methods for doing it.

2) I think we should treat http and https as completely different. There are many bugs that come from treating them as the same. We should just the 'same origin' protocol here.

In addition, the original code doesn't handle things like scheme relative URLs correctly. I'll come up with a patch shortly.

comment:4 Changed 3 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from new to closed

In [16183]:

Fixed #15869 - example AJAX code in CSRF docs fails sometimes for IE7 or absolute same origin URLs

Thanks to nick for the report.

comment:5 Changed 3 years ago by lukeplant

In [16184]:

[1.3.X] Fixed #15869 - example AJAX code in CSRF docs fails sometimes for IE7 or absolute same origin URLs

Thanks to nick for the report.

Backport of [16183] from trunk.

comment:6 Changed 3 years ago by lukeplant

In [16185]:

[1.2.X] Fixed #15869 - example AJAX code in CSRF docs fails sometimes for IE7 or absolute same origin URLs

Thanks to nick for the report.

Backport of [16183] from trunk.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.