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

MM-14052: Fix subpath rewrite again #10252

Merged
merged 2 commits into from Feb 12, 2019

Conversation

Projects
None yet
6 participants
@lieut-data
Copy link
Member

lieut-data commented Feb 8, 2019

Summary

The subpath rewrite feature broke in v5.8, as the Content-Security-Policy tag in the root.html changed once again, this time to drop the 'unsafe-eval' directive. The rewrite feature itself is brittle to these kinds of changes precisely to avoid accidentally changing things, and it was expected that root.html would change infrequently (especially on this line!).

This PR fixes the regular expressions used to match the current value of the CSP directive. Note that we can't fix this at build time, since the rewrite occurs on server startup and is driven by the SiteURL. We could avoid some of this pain if we hard-coded an extra XHR request on startup to fetch the subpath assets location, but this is non-trivial and needlessly slows down all installations for a relatively minor pain that has always been caught in QA.

This PR also adds a missing log that would have immediately revealed the failure instead of requiring spelunking in the server itself.

Open to suggestions for removing the brittleness!

Ticket Link

https://mattermost.atlassian.net/browse/MM-14052

Checklist

  • Added or updated unit tests (required for all new features)

lieut-data added some commits Feb 8, 2019

@lieut-data lieut-data added this to the v5.8.0 milestone Feb 8, 2019

@lieut-data lieut-data requested review from jwilander and deanwhillier Feb 8, 2019

@lieut-data lieut-data changed the title Fix subpath rewrite again MM-14052: Fix subpath rewrite again Feb 8, 2019

@lieut-data

This comment has been minimized.

Copy link
Member Author

lieut-data commented Feb 8, 2019

@deanwhillier, I've added you given your recent interest in Webpack ;P. If you want an overview of how subpaths work, check out https://developers.mattermost.com/blog/subpath/.

@jwilander
Copy link
Member

jwilander left a comment

LGTM. +1 on adding the log

@deanwhillier
Copy link

deanwhillier left a comment

LGTM

@deanwhillier
Copy link

deanwhillier left a comment

Upon further discussion, we need to update the subpath functionality to account for favicon paths that are now embedded in the root.html template file.

Relevant lines in root.html (17-19):

    <link rel="icon" type="image/png" href="/static/images/favicon/favicon-16x16.png" sizes="16x16">
    <link rel="icon" type="image/png" href="/static/images/favicon/favicon-32x32.png" sizes="32x32">
    <link rel="icon" type="image/png" href="/static/images/favicon/favicon-96x96.png" sizes="96x96">
@lieut-data

This comment has been minimized.

Copy link
Member Author

lieut-data commented Feb 12, 2019

@deanwhillier and I chatted and tested this offline -- I'd forgotten that, indeed, the subpath rewriting handles any /static/* assets. Confirmed that the favicon gets rewritten.

@lieut-data lieut-data merged commit aca8914 into master Feb 12, 2019

3 checks passed

continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
mattermost/serverless-ops/github PR is up-to-date.

@lieut-data lieut-data deleted the fix-subpath-rewrite-again branch Feb 12, 2019

lieut-data added a commit that referenced this pull request Feb 12, 2019

MM-14052: Fix subpath rewrite again (#10252)
* actually log an error when subpath rewrite fails

* update subpath rewrite to accommodate dropping unsafe-eval
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment