Edgewall Software
Modify

Opened 13 years ago

Closed 11 years ago

Last modified 11 years ago

#7768 closed defect (fixed)

add_script & add_stylesheet don't support external scripts

Reported by: martin@… Owned by: Remy Blank
Priority: high Milestone: 1.0
Component: web frontend Version:
Severity: normal Keywords: add_script
Cc: ryano@… Branch:
Release Notes:

API only

API Changes:

trac.web.chrome: add_script() and add_stylesheet() now also accept absolute URLs [10027]

Internal Changes:

Description

Not sure if this was indented as a security feature, but:

The add_script from trac.web.chrome doesn't support external script URLs, i.e. one which start with 'http://'. At the moment the full URL is appended to the chrome path resulting in a dysfunctional script tag.

The same should be true for add_stylesheet.

If this is indented as a security feature, then be warned that it is easily overcome by just coping and changing the add_script source code, like I did in my GoogleMapMacro, where I have to load the external Google Map API JavaScript file.

Attachments (1)

allow_absolute_urls_for_add_script_add_stylesheet.patch (1.1 KB ) - added by mark.m.mcmahon@… 11 years ago.
Adds a check for the filename starting with http

Download all attachments as: .zip

Change History (9)

comment:1 by Christian Boos, 13 years ago

Component: generalweb frontend
Keywords: add_script added
Milestone: 0.13

Well, it's certainly not a "security feature", as you can always to whatever you want in plugins ;-)

comment:2 by Christian Boos, 11 years ago

comment:3 by mark.m.mcmahon@…, 11 years ago

Not really sure if this patch is correct or not - but adding it here for feedback/improvements.

by mark.m.mcmahon@…, 11 years ago

Adds a check for the filename starting with http

comment:4 by Remy Blank, 11 years ago

Milestone: next-major-0.1X0.13
Owner: set to Remy Blank

Looks good.

comment:5 by Ryan J Ollos <ryano@…>, 11 years ago

Cc: ryano@… added

comment:6 by Remy Blank, 11 years ago

Priority: normalhigh

comment:7 by Remy Blank, 11 years ago

Resolution: fixed
Status: newclosed

… well, not quite good :) Your patch doesn't define href in the "absolute URL" case.

I have applied an improved version in [10027], together with updated unit tests that check the correct behavior.

Mark, I hope you don't mind if I give you some advice for you next patches:

  • Always run the unit tests and functional tests before posting your patch. This should catch most of the more obvious errors.
  • If possible, add one or more unit tests (or modify existing tests) to check for the correctness of your changes.
  • Always create your patches from the project root, so that full paths are recorded. This makes it simpler for the reviewer to apply them.

But most importantly: keep up the good work!

comment:8 by Christian Boos, 11 years ago

API Changes: modified (diff)
Release Notes: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.