Thimble needs to warn users when there are http links in https document

RESOLVED FIXED

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cassie, Assigned: xdchen)

Tracking

Details

(Whiteboard: uxdesign, mozfest, URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
I've experienced this myself and in the workshop I hosted last night.

If users want to link to an http document (like google fonts - the embed instructions use an http link), it doesn't work when you are working on an Https page (until you change the link url also to https).

Since it is such a simple thing to fix, but is a problem that would really stump beginners, is there a way to set this as a warning, or somehow let them know why it isn't working (even though they are following the google font instructions perfectly)?
:Pomax, this looks like it might make for a good student bug, maybe have these errors pop up as slowparse warnings or errors? Thoughts?
Flags: needinfo?(pomax)

Comment 2

6 years ago
jbuck: that sounds like a great idea. Let's add it as a place for students to jump in.
Flags: needinfo?(pomax)
(Assignee)

Comment 3

6 years ago
I would like to make a try, can somebody give me a starting point?

Comment 4

6 years ago
There's a few ways to do this, the most interesting is probably to add a new error case to the slowparse library and add a special handler for the "href" attribute (which will implicitly catch <a> and <link> elements), https://github.com/mozilla/slowparse/blob/gh-pages/slowparse.js#L1228

Comment 5

6 years ago
note: parse errors are define on https://github.com/mozilla/slowparse/blob/gh-pages/slowparse.js#L98 and on, and you would probably add your own code after https://github.com/mozilla/slowparse/blob/gh-pages/slowparse.js#L1253, which is where the code is technically valid HTML and so might be a legal (but mixed-content) href="..." pair.
(Assignee)

Comment 6

6 years ago
code is ready, will setup local thimble to test it
(Assignee)

Comment 7

6 years ago
After bug fixing, shall I build the package locally to make it work? how to build it?
(Assignee)

Comment 8

6 years ago
Now start to test the basic function (locate href="http://..." and trigger error), but still one question, how to run thimble in a local https environment.
We brought this bug up in a team meeting today and would love to ship for our mozfest milestone.

Adding Pomax to help with local install questions
Assignee: nobody → pomax
Flags: needinfo?(pomax)
Whiteboard: uxdesign, → uxdesign, mozfest
Note that dchen has been working on this, so maybe Pomax wants to co-ordinate vs. take.

Comment 11

6 years ago
sounds good, dchen, how far did you get?
Flags: needinfo?(pomax)
(Assignee)

Comment 12

6 years ago
Hi, Mike

I'm still working on it. The http links can be located, but trigger ParseError failed. If you have time, please have a look at my repo:
 https://github.com/xdchen/slowparse/commit/3b3146bc8188d907a4c534bee28e64feb393675d
(Assignee)

Comment 13

6 years ago
update the warning trigger with tag-attributes pairs (like link-href, a-href, iframe-src etc).
https://github.com/xdchen/slowparse/commit/15ca7d8281765c1fc10af3f21f4cbd48db6e0cc7

Updated

6 years ago
Assignee: pomax → dchen.xd

Comment 14

6 years ago
if you make this a pull request, and set me to r?, I can leave code comments.
(Assignee)

Comment 15

6 years ago
Bug fixed, please review the pull request.
https://github.com/mozilla/slowparse/pull/56
Flags: needinfo?(pomax)
xdchen, you need to flag someone for review.  I've done it for you, but you can do it yourself next time.  Great work on this patch!
Attachment #818179 - Flags: review?(pomax)
Flags: needinfo?(pomax)
(Assignee)

Comment 17

6 years ago
Thank you for the help, David.

Comment 18

6 years ago
Comment on attachment 818179 [details] [review]
https://github.com/mozilla/slowparse/pull/56

pretty nice solution, although we're missing a global check to see if slowparse itself is being run on http or https (if it's on http, we don't need to do mixed content checking). I left some comments in the pull request, set me back to R? when you've addressed then and I'll rereview =)
Attachment #818179 - Flags: review?(pomax) → review-
(Assignee)

Comment 19

6 years ago
Hi, Mike

For the https status, I just added the checkMixedContent check follow your suggestion, but I can not test it locally on http://localhost:3500/. 

Please have a review.
Attachment #818647 - Flags: review?(pomax)

Comment 20

6 years ago
I'll have a look, but you don't have to make a new attachment when you want a rereview, just go into the "details" link for the original attachment, and change the review flag from "-" to "?" and add the reviewer again. This also lets you add a comment for the details you changed.

Comment 21

6 years ago
Comment on attachment 818647 [details] [review]
Updated with JavaScript code conventions + https check

we'll need to move the http vs. https check up to the beginning of the slowparse function definition, so that it's a global property that's only set once, rather than every time we call functions. There's also two easy to fix small issues left in the other code.
Attachment #818647 - Attachment filename: file_911169.txt → 56
Attachment #818647 - Flags: review?(pomax) → review-
(Assignee)

Comment 22

6 years ago
Comment on attachment 818647 [details] [review]
Updated with JavaScript code conventions + https check

move checkMixedContent to very beginning of the file + indentation improvement.
Attachment #818647 - Flags: review- → review?(pomax)

Comment 23

6 years ago
Comment on attachment 818647 [details] [review]
Updated with JavaScript code conventions + https check

R+, provided you add a few newlines for visual separation. Other than that, we're good. If you add those in, and then rebase your patch so it's a single commit, I can merge it into thimble for you.
Attachment #818647 - Flags: review?(pomax) → review+
(Assignee)

Comment 24

6 years ago
Another question, shall we add (img/src) into isActiveContent?
(Assignee)

Comment 25

6 years ago
Posted file Rework on a new clean branch (obsolete) —
Please review the pull request https://github.com/mozilla/slowparse/pull/57, and remove the last one (56), thank you
Attachment #819241 - Flags: review?(pomax)

Comment 26

6 years ago
images are not active content.
(Assignee)

Comment 27

6 years ago
ok, I will remove img-src

Comment 28

6 years ago
Comment on attachment 819241 [details] [review]
Rework on a new clean branch

test and works pretty nicely! I think we need to change the error message to be more informative on what's gone wrong and how users can fix it (suggested text is in the PR), but with that changed, this can land.
Attachment #819241 - Flags: review?(pomax) → review-
(Assignee)

Comment 29

6 years ago
Comment on attachment 819241 [details] [review]
Rework on a new clean branch

error message updated.
Attachment #819241 - Flags: review- → review?(pomax)

Comment 30

6 years ago
Comment on attachment 819241 [details] [review]
Rework on a new clean branch

very nice work!
Attachment #819241 - Flags: review?(pomax) → review+

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 31

6 years ago
merged into slowparse, but thimble does not use slowparse as submodule anymore, so we'll need to update the slowparse.js and the errors.base.html files in the thimble.webmaker.org repository, too.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
If we can get this done in Thimble before Monday at 6pm, we can ship this for Mozfest.  It would be great if that were possible.

This is a great fix, thanks for working on it.

Comment 33

6 years ago
updated for slowparse in thimble
Attachment #818179 - Attachment is obsolete: true
Attachment #818647 - Attachment is obsolete: true
Attachment #819241 - Attachment is obsolete: true
Attachment #819494 - Flags: review?(ali)

Comment 34

6 years ago
@humph: yeah this is going to pop us so much at MozFest, having it in will make everyone's experience a lot better!
Comment on attachment 819494 [details]
https://github.com/mozilla/thimble.webmaker.org/pull/259/files

What an amazing patch!!

R+ - อาร์พลัส
Attachment #819494 - Flags: review?(ali) → review+

Comment 36

6 years ago
Commit pushed to master at https://github.com/mozilla/thimble.webmaker.org

https://github.com/mozilla/thimble.webmaker.org/commit/d0d16e099c05bfeb165f111279a9887a60056cbd
Merge pull request #259 from Pomax/bug911169

in the mixed content world, we are now all winners!

Comment 37

6 years ago
landed!
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 38

6 years ago
Thank you Mike and David, I learned a lot from you during the process.
(Assignee)

Comment 39

6 years ago
(In reply to Mike "Pomax" Kamermans [:pomax] from comment #31)
> merged into slowparse, but thimble does not use slowparse as submodule
> anymore, so we'll need to update the slowparse.js and the errors.base.html
> files in the thimble.webmaker.org repository, too.

shall I do it?
Yeah, if you could get that patch ready that would be awesome
(Assignee)

Comment 41

6 years ago
I didn't notice that :pomax have done the merge in thimble.webmaker.org repository as pull request 259, and duplicated the works in pull request 261.

so please cancel it:

https://github.com/mozilla/thimble.webmaker.org/pull/261
You need to log in before you can comment on or make changes to this bug.