fix: js-sdk directory/file permission should be set correctly #3616
Conversation
|
I'm surprised that this wasn't caught by the tests. We should look into that too. |
Is it not? |
Don't know? If it is, we should not have this issue shall we? At least that's what I get from the original issue report? |
Co-authored-by: Amin Vakil <info@aminvakil.com>
|
Ah we got this on CI |
install/setup-js-sdk-assets.sh
Outdated
| @@ -34,7 +34,12 @@ if [[ "${SETUP_JS_SDK_ASSETS:-}" == "1" ]]; then | |||
| variants="{bundle,bundle.tracing,bundle.tracing.replay,bundle.replay,bundle.tracing.replay.feedback,bundle.feedback}" | |||
|
|
|||
| # Download those versions & variants using curl | |||
| $dcr --no-deps --rm -v "sentry-nginx-www:/var/www" nginx curl -w '%{response_code} %{url}\n' --no-progress-meter --compressed --retry 3 --create-dirs -fLo "/var/www/js-sdk/#1/#2.min.js" "https://browser.sentry-cdn.com/${versions}/${variants}.min.js" || true | |||
| $dcr --no-deps --rm -v "sentry-nginx-www:/var/www" --user=nginx nginx curl -w '%{response_code} %{url}\n' --no-progress-meter --compressed --retry 3 --create-dirs -fLo "/var/www/js-sdk/#1/#2.min.js" "https://browser.sentry-cdn.com/${versions}/${variants}.min.js" || true | |||
There was a problem hiding this comment.
What do you think of adding this prior to downloading files by nginx user?
$dcr --no-deps --rm -v "sentry-nginx-www:/var/www" nginx mkdir -p "/var/www/js-sdk/#1"
|
Okay just checked mine. It's owned by
|
|
The release window is closing, let's just choose the easy route for now. |
| @@ -27,14 +27,20 @@ if [[ "${SETUP_JS_SDK_ASSETS:-}" == "1" ]]; then | |||
| latest_js_v6=$(echo "$loader_registry" | $jq -r '.versions | reverse | map(select(.|any(.; startswith("6.")))) | .[0]') | |||
| latest_js_v7=$(echo "$loader_registry" | $jq -r '.versions | reverse | map(select(.|any(.; startswith("7.")))) | .[0]') | |||
| latest_js_v8=$(echo "$loader_registry" | $jq -r '.versions | reverse | map(select(.|any(.; startswith("8.")))) | .[0]') | |||
| latest_js_v9=$(echo "$loader_registry" | $jq -r '.versions | reverse | map(select(.|any(.; startswith("9.")))) | .[0]') | |||
There was a problem hiding this comment.
well you just broke unit tests to start? :D
Head branch was pushed to by a user without write access
_unit-test/js-sdk-assets-test.sh
Outdated
| test "23" == "$non_empty_file_count" | ||
| echo "Pass" | ||
|
|
||
| # Files should be owned by the nginx user |
There was a problem hiding this comment.
| # Files should be owned by the nginx user | |
| # Files should be owned by the root user |
Head branch was pushed to by a user without write access
|
Since 25.3.0 is released and this is not merged yet. Let's think of a proper solution instead. |
|
@aldy505 still merged as it fixes a problem and adds a test for the issue. Trusting you to follow up with a better solution. |
Closes #3614
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.