Ticket #7768 (closed defect: fixed)
Opened 3 years ago
Last modified 17 months ago
add_script & add_stylesheet don't support external scripts
| Reported by: | martin@… | Owned by: | rblank |
|---|---|---|---|
| Priority: | high | Milestone: | 0.13 |
| Component: | web frontend | Version: | |
| Severity: | normal | Keywords: | add_script |
| Cc: | ryano@… | ||
| Release Notes: |
API only |
||
| API Changes: |
trac.web.chrome: add_script() and add_stylesheet() now also accept absolute URLs [10027] |
||
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
Change History
comment:1 Changed 3 years ago by cboos
- Component changed from general to web frontend
- Keywords add_script added
- Milestone set to 0.13
comment:2 Changed 19 months ago by cboos
comment:3 Changed 18 months ago by mark.m.mcmahon@…
Not really sure if this patch is correct or not - but adding it here for feedback/improvements.
Changed 18 months ago by mark.m.mcmahon@…
- Attachment allow_absolute_urls_for_add_script_add_stylesheet.patch added
Adds a check for the filename starting with http
comment:4 Changed 18 months ago by rblank
- Milestone changed from next-major-0.1X to 0.13
- Owner set to rblank
Looks good.
comment:5 Changed 18 months ago by Ryan J Ollos <ryano@…>
- Cc ryano@… added
comment:6 Changed 18 months ago by rblank
- Priority changed from normal to high
comment:7 Changed 18 months ago by rblank
- Resolution set to fixed
- Status changed from new to closed
... 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!
Well, it's certainly not a "security feature", as you can always to whatever you want in plugins ;-)