Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #423

Closed
wants to merge 2 commits into from
Closed

Fixes #423

wants to merge 2 commits into from

Conversation

Zren
Copy link
Contributor

@Zren Zren commented Nov 15, 2014

  • Make express-minify an optional dependancy as we only use it in production and it requires a compiler to install (pain). Also make morgan optional even though it doesn't need a compiler. I could probably make it a devDependancy but it's not terribly critical (as we might want to use it in production).
  • Remove Icons from non https:// or data:image/ URIs (png/gif only atm, probably should add jpg or just allow anything).

Zren added 2 commits November 15, 2014 14:19
express-minify requires a compiler to install so making it optional makes setting up a new dev env easier.
@Martii
Copy link
Member

Martii commented Nov 15, 2014

express-minify ... it requires a compiler to install (pain).

Installs fine for me... what do you mean?

Also make morgan optional even though it doesn't need a compiler.
Zren@719540a

Request logging will eventually occur on production so -1 for this... plus I mentioned to you to keep require statements at the top for now.

Delete non https/data:image URI's during Script parsing.
Zren@3649939

Interesting idea but why not let the end user decide instead with their own sec policy?

@Zren
Copy link
Contributor Author

Zren commented Nov 15, 2014

Installs fine for me... what do you mean?

My VS compiler install is messed up and have been putting off fixing it. In any case, it requires node-sass, which requires node-gyp to build something, which requires visual studios. There was a reason we went with javascript toobusy-js instead of the C version. Since it's not used for development anyways, may as well make it optional.

Request logging will eventually occur on production so -1 for this....

No big deal. It was to be slightly consistent with optional stuff anyways.

Interesting idea but why not let the end user decide instead with their own sec policy?

Because the user should not have to make that decision. If he's given an option, he'll be scared away / choose the more secure option anyways. Greasyfork doesn't allow images from http, so conforming with them would be best.

@jerone
Copy link
Contributor

jerone commented Nov 15, 2014

@Zren commented on 15 nov. 2014 23:17 CET:

Installs fine for me... what do you mean?

My VS compiler install is messed up and have been putting off fixing it. In any case, it requires node-sass, which requires node-gyp to build something, which requires visual studios. There was a reason we went with javascript toobusy-js instead of the C version. Since it's not used for development anyways, may as well make it optional.

Can you elaborate on this. I'm using VS for development too and have no problems with it.

@Zren commented on 15 nov. 2014 20:37 CET:

  • Remove Icons from non https:// or data:image/ URIs (png/gif only atm, probably should add jpg or just allow anything).

I was thinking about why not hide the non-https images when they are viewed on https site...

@Zren
Copy link
Contributor Author

Zren commented Nov 15, 2014

I was thinking about why not hide the non-https images when they are viewed on https site...

Forgot we actually support http://. So you want me to strip the http: out of urls then?

@jerone
Copy link
Contributor

jerone commented Nov 15, 2014

@Zren commented on 15 nov. 2014 23:50 CET:

I was thinking about why not hide the non-https images when they are viewed on https site...

Forgot we actually support http://. So you want me to strip the http: out of urls then?

I was more thinking about not showing the images in the views when https:// only.
But this might require some more discussion. You probably did this because of the mixed content issue, right?

@Martii
Copy link
Member

Martii commented Nov 15, 2014

... this mite require some more discussion.

The problem is with google doing false positives of this... I'd be more interested if authed then just do it and allow the user to control their own security policy (we don't need to be a dictator which is one of sizzles previously mentioned goals and one of mine on USO.)... if not authed google would either get a (possible) 404 which would hurt our SEO ranking in which case we should just show the "default" icon (which is actually a font... maybe with another unique indicator).


This still doesn't address #user-content, .user-data, and a rare chance of #document-content space... in which I will reiterate from earlier in another issue that google is to blame for that false positive and they should have "attacked" that site (theirs) instead of ours.

@Martii
Copy link
Member

Martii commented Nov 18, 2014

@Zren express-minify is currently on Pro and Dev now but not Dbg (debug) ... basically if you are having issues with this it would be best to resolve them. My solution was 3 fold... upgrade my distro... compile my own node instead of using my distros... and then installing a newer npm. toobusy package currently compiles okay here now but of course we aren't using it anymore.

@Zren
Copy link
Contributor Author

Zren commented Nov 19, 2014

But this mite require some more discussion. You probably did this because of the mixed content issue, right?

Yep. Mainly because I thought the chrome was blocking mixed content over HTTPS but that was the SSL cert. I'll put this on the backburner for now.

@Zren express-minify is currently on Pro and Dev now but not Dbg (debug) ... basically if you are having issues with this it would be best to resolve them.

Don't really want to fix my C compiler until I reformat & reinstall Windows, which I'm not planning on doing until I get a new drive (possible an SSD :] ). So making my current env work until then. Part of what got me to actually help with OUJS is how easy it was to setup a dev env. It didn't require a compiler (after we made it optional, and eventually used toobusy-js instead). I didn't have to setup MongoDB as the defaults point to the dev db. All I had to do was clone + npm install + run.

Going to make a new PR without the HTTPS commit and just make express-minify optional.

@Zren Zren closed this Nov 20, 2014
@Martii
Copy link
Member

Martii commented Nov 20, 2014

SSD's are quite an improvement... they are worth it if not too expensive.

until I reformat & reinstall Windows

Windows is giving you a compiling error? Most of node under that is precompiled last I installed about a little over a week ago... sounds to me like you have bigger issues if Windows is doing this to you.


Forward ref #431

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants