fix: missing mime types and turning off autoindex for js-sdk endpoint #3395

aldy505 · 2024-10-25T06:33:10Z

Things weren't as smooth as I thought it would be.

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

codecov · 2024-10-25T06:46:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.01%. Comparing base (49e30a7) to head (a95f678).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3395      +/-   ##
==========================================
+ Coverage   98.02%   99.01%   +0.98%     
==========================================
  Files           3        3              
  Lines         203      203              
==========================================
+ Hits          199      201       +2     
+ Misses          4        2       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aminvakil

Avoid adding a custom mime.types.

Hanno Böck has a great talk regarding this matter and different security implications about this if you're interested:
https://www.youtube.com/watch?v=8t8JYpt0egE

Just use /etc/nginx/mime.types which already gets shipped in all nginx installations AFAIK.

And what do you say we keep each PR on changing one thing?

aminvakil · 2024-10-25T10:55:32Z

nginx/mime.types

@@ -0,0 +1,99 @@
+


There is mime.types present in all nginx instances in /etc/nginx/mime.types and it should be included by default. It oddly seems that is has never been included in self-hosted nginx configurations, and old onpremise instances.

I'm so glad I ask your review for this 😀. I'll update the docker compose instead and remove this.

aminvakil · 2024-10-25T10:56:33Z

nginx/nginx.conf

@@ -89,8 +89,12 @@ http {
 			proxy_pass http://relay;
 		}
 		location ^~ /js-sdk/ {
-			autoindex on;
-			root  /var/www/js-sdk;
+			include mime.types;


It's better to just remove this and include this on all blocks (in http block).

aminvakil · 2024-10-25T10:58:09Z

nginx/nginx.conf

+			autoindex off;
+			etag on;
+			root  /var/www/;
+			try_files $uri =404;


What does this tries to do? nginx will return 404 by default if there is a file missing.

aminvakil · 2024-10-25T10:59:51Z

nginx/nginx.conf

+			etag on;
+			root  /var/www/;
+			try_files $uri =404;
+			add_header Access-Control-Allow-Origin *;


I don't know the reason of this, but I will go through issues and PRs to figure out what has been broken, but generally this is a bad practice, although may be necessary in some situations, sorry I'm not in the loop completely about js-sdk, just my two cents on this.

The original sentry cdn has this for some reason. I'm trying to make it as close as possible, so there's that.

aminvakil · 2024-10-25T13:21:47Z

nginx/nginx.conf

+			etag on;
+			root  /var/www/;
+			try_files $uri =404;
+			add_header Access-Control-Allow-Origin *;


Yeah, probably because sentry.io cannot add all domains and subdomains users are using. Although I'm not sure, and if it responds to any random subdomain which users point to sentry.io.

Anyway I'd suggest at least adding a comment stating that it's recommended for users to change this.

Suggested change

add_header Access-Control-Allow-Origin *;

add_header Access-Control-Allow-Origin *; # it is recommended to change this to your sentry (in most cases same as system.url-prefix)

aminvakil · 2024-10-25T13:29:33Z

nginx/nginx.conf

-			root  /var/www/js-sdk;
+			include mime.types;
+			autoindex off;
+			etag on;


Nginx enables etag by default for static contents and 99.99% it keeps on doing this.

aminvakil · 2024-10-25T13:30:19Z

nginx/nginx.conf

-			autoindex on;
-			root  /var/www/js-sdk;
+			include mime.types;
+			autoindex off;


Same as etag.

aldy505 · 2024-10-26T11:24:49Z

So the previous configuration won't resolve the path properly and every Javascript assets file will always returns Content-Type of application/octet-stream, which.. is not what we're expecting.

It also crosses my mind that we should probably update the nginx version?

aminvakil · 2024-10-27T12:15:38Z

docker-compose.yml

@@ -457,8 +457,8 @@ services:
    volumes:
      - type: bind
        read_only: true
-        source: ./nginx
-        target: /etc/nginx
+        source: ./nginx/nginx.conf


docker-compose makes some problems bind mounting a single file, I'd suggest not changing this.

Then /etc/nginx/mime.types wouldn't exists on that directory. Do you have any suggestion for this?

No :)

docker-compose makes some problems bind mounting a single file, I'd suggest not changing this.

It's OK here though as I think more about it. No other process is going to use this file and no other process is going to change this file anyway.

Shall we mount the directory to some other path and do something like this instead then: https://stackoverflow.com/a/30152496/90297

As it would be quite confusing for someone to realize nginx is the odd one out where we only mount a single file in case we want to add something new in the future?

That would be a worse hack than this IMHO.

see some issues that people can't build their own Docker image since they're behind some sort of hard network proxy/firewall

I wasn't recommending us to build our own image, you can override CMD and ENTRYPOINT from docker-compose.yml.

Anyway, may I offer a compromise then:

Move the nginx.conf file outside of the nginx directory

Mount that file by itself

It achieves what we want both: hassle-free and not confusing for any onlookers or future maintainers.

WDYT @aldy505?

1. Move the `nginx.conf` file outside of the `nginx` directory

Where do you think would be suitable? I think the appropriate directory would be nginx directory at last.

My proposal is to move to the top level instead of a dedicated directory to avoid confusion around mounting.

This is the best way to go IMHO as well.

Alright, will do that.

nginx/nginx.conf

Co-authored-by: Amin Vakil <[email protected]>

aminvakil

LGTM

aminvakil · 2024-10-27T12:53:38Z

It also crosses my mind that we should probably update the nginx version?

I'd say we can keep that for another PR, but also fine with bumping it here.

BYK · 2024-10-28T16:50:04Z

nginx/nginx.conf

-			autoindex on;
-			root  /var/www/js-sdk;
+			root /var/www/;
+			add_header Access-Control-Allow-Origin *; # It is recommended to change this to your sentry (in most cases same as system.url-prefix)


nit: Would put the comment above the line for readability.

This has not been fixed yet.

aldy505 · 2024-11-06T01:08:17Z

Ping @hubertdeng123 we should be good to go now

BYK

Rejecting for two pending comments

fix: missing mime types and turning off autoindex for js-sdk endpoint

9f9565b

aldy505 requested review from BYK, aminvakil and hubertdeng123 October 25, 2024 06:33

aminvakil suggested changes Oct 25, 2024

View reviewed changes

aminvakil reviewed Oct 25, 2024

View reviewed changes

fix: remove mime.types, only bind to nginx.conf

ddba719

aldy505 requested a review from aminvakil October 26, 2024 11:23

aminvakil reviewed Oct 27, 2024

View reviewed changes

Update nginx/nginx.conf

f15439f

Co-authored-by: Amin Vakil <[email protected]>

aminvakil approved these changes Oct 27, 2024

View reviewed changes

BYK reviewed Oct 28, 2024

View reviewed changes

Merge branch 'master' into fix/js-sdk-nginx-config

907ecce

BYK requested changes Nov 6, 2024

View reviewed changes

aldy505 added 2 commits November 7, 2024 07:58

chore: move nginx.conf to top level

c595a7b

chore: provide comment explaining on Access-Control-Allow-Origin header

713c03d

aldy505 requested review from BYK, aminvakil and joshuarli November 7, 2024 01:01

Merge branch 'master' into fix/js-sdk-nginx-config

e3bdb3e

BYK approved these changes Nov 7, 2024

View reviewed changes

BYK enabled auto-merge (squash) November 7, 2024 10:14

aldy505 disabled auto-merge November 7, 2024 10:21

chore: mixed space and tabs

583a13b

aminvakil approved these changes Nov 7, 2024

View reviewed changes

Merge branch 'master' into fix/js-sdk-nginx-config

a95f678

BYK merged commit 98f6cf0 into getsentry:master Nov 7, 2024
11 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: missing mime types and turning off autoindex for js-sdk endpoint #3395

fix: missing mime types and turning off autoindex for js-sdk endpoint #3395

aldy505 commented Oct 25, 2024

codecov bot commented Oct 25, 2024 •

edited

Loading

aminvakil left a comment •

edited

Loading

aminvakil Oct 25, 2024

aldy505 Oct 25, 2024

aminvakil Oct 25, 2024

aminvakil Oct 25, 2024

aminvakil Oct 25, 2024

aldy505 Oct 25, 2024

aminvakil Oct 25, 2024

aminvakil Oct 25, 2024

aminvakil Oct 25, 2024

aldy505 commented Oct 26, 2024

aminvakil Oct 27, 2024

aldy505 Oct 27, 2024

aminvakil Oct 27, 2024

BYK Oct 28, 2024

aminvakil Oct 28, 2024

BYK Nov 6, 2024

aminvakil Nov 6, 2024

BYK Nov 6, 2024

aminvakil Nov 6, 2024

aldy505 Nov 7, 2024

aminvakil left a comment

aminvakil commented Oct 27, 2024

BYK Oct 28, 2024

aminvakil Nov 6, 2024

aldy505 commented Nov 6, 2024

BYK left a comment

	add_header Access-Control-Allow-Origin *;
	add_header Access-Control-Allow-Origin *; # it is recommended to change this to your sentry (in most cases same as system.url-prefix)

fix: missing mime types and turning off autoindex for js-sdk endpoint #3395

fix: missing mime types and turning off autoindex for js-sdk endpoint #3395

Conversation

aldy505 commented Oct 25, 2024

Legal Boilerplate

codecov bot commented Oct 25, 2024 • edited Loading

Codecov Report

aminvakil left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldy505 commented Oct 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aminvakil left a comment

Choose a reason for hiding this comment

aminvakil commented Oct 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldy505 commented Nov 6, 2024

BYK left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 25, 2024 •

edited

Loading

aminvakil left a comment •

edited

Loading