Opened 6 years ago

Closed 5 years ago

#17535 closed defect (fixed)

[patch][cla] dojox/html/styles throws SecurityError

Reported by: Wouter Hager Owned by: dylan
Priority: low Milestone: 1.10
Component: Dojox Version: 1.9.1
Keywords: Cc:
Blocked By: Blocking:

Description

A same origin violation error is thrown when dojox/html/styles encounters a stylesheet from another domain.

dojox/data/CssRuleStore has the same problem, see #16096

Attachments (1)

if-remote-cssRules-fails.patch (964 bytes) - added by Wouter Hager 5 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 5 years ago by bill

Component: GeneralDojox
Owner: set to Adam Peller

Changed 5 years ago by Wouter Hager

comment:2 Changed 5 years ago by Wouter Hager

please review attached patch

comment:3 Changed 5 years ago by bill

FYI, patches should be submitted as pull requests, see https://github.com/dojo/dojo/blob/master/CONTRIBUTING.md#submitting-a-pull-request.

In this case though I don't see the point of this change. All you are doing is silently surpressing an error message. The stylesheet load still doesn't work.

comment:4 Changed 5 years ago by Wouter Hager

Apologies, I would be happy to create PRs as it's less of a hassle, but I thought I had to provide a patch. And apologies again, I had a working fix, but made a small edit without testing. I'll try again later today.

comment:6 Changed 5 years ago by Wouter Hager

Hi Bill, I'm not quite sure what you expect. Catching the silent error means that the script will just continue loading other stylesheets, won't it?

comment:7 Changed 5 years ago by bill

That's what I would guess. Do you think that's a good thing? Isn't it better to tell the app developer that they have a problem and they need to fix it?

comment:8 Changed 5 years ago by Wouter Hager

I'd warn about it in the console. That way the developer at least gets some info. A solution would be to add a CORS attribute to the <link> element, but that's HTML5, so I can't put that in the warning.

I think files loaded from CDN should be ignored anyway.

So all in all I think it's a good enough idea.

comment:9 Changed 5 years ago by dylan

Milestone: tbd1.10
Summary: dojox/html/styles throws SecurityError[patch][cla] dojox/html/styles throws SecurityError

comment:10 Changed 5 years ago by Wouter Hager

Hm well maybe you're right. But the error shouldn't be silent. I'll try to find out why it is.

comment:11 Changed 5 years ago by bill

I definitely agree that the error shouldn't be silent. I'm not familiar with the code you are changing; perhaps the error occurs in a separate thread, asynchronously?

comment:12 Changed 5 years ago by Wouter Hager

My mistake, it's not silent. Sorry, don't know why I thought it was.

comment:13 Changed 5 years ago by Wouter Hager

Ah I understand, the error is silent in dojox/data/css.js. see #16096

That's probably why I filed this in the first place, I must have mixed up the issues.

comment:14 Changed 5 years ago by bill

OK, so this ticket (and associated pull request) should be closed?

comment:15 Changed 5 years ago by Wouter Hager

Well I stil think a warning (and more verbose) would be better. Not allowed to read one css doesn't mean all should fail. This is a convenience package to begin with. But that is my opinion, so I leave it up to you.

Last edited 5 years ago by Wouter Hager (previous) (diff)

comment:16 Changed 5 years ago by dylan

Owner: changed from Adam Peller to dylan
Priority: undecidedlow
Status: newassigned

@wshager, at this point, I think you know this code as much as anyone that's active, so if you have an alternative patch to propose, we'll consider it. Thanks!

comment:17 Changed 5 years ago by Wouter Hager

I've added a warning. I think this convenience should be convenient.

comment:18 Changed 5 years ago by Wouter Hager

Please note that CSS from CDN (ajax.googleapis.com/ajax/libs/dojo) prevents mimetype sniffing. That probably causes the issue in my code.

Last edited 5 years ago by Wouter Hager (previous) (diff)

comment:19 Changed 5 years ago by bill

I'm fine with any changes, you can change it to a warning if you like. I guess throwing exceptions doesn't work if you are in a separate thread (i.e. an asynchronous callback).

comment:20 Changed 5 years ago by dylans <dylan@…>

Resolution: fixed
Status: assignedclosed

In 8f4fb8519532b1a02ee8043eddb24852e8222833/dojox:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 
Note: See TracTickets for help on using tickets.