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

Append version string to resources to enforce reload after updating #2367

Closed
Kimmax opened this issue Apr 19, 2018 · 13 comments
Closed

Append version string to resources to enforce reload after updating #2367

Kimmax opened this issue Apr 19, 2018 · 13 comments
Assignees
Labels
cleanup Low impact changes
Milestone

Comments

@Kimmax
Copy link

Kimmax commented Apr 19, 2018

TL;DR:
Add v=<Version> to script and style resources to force reload them after updating opnsense, while keeping the caching functionality of browsers intact.

I ran into cache problems after a script was updated and my browser refused to load the new one. While this is not a big problem, it's a known one that can be easily mitigated by appending a v=<Version> string to the URL's of said resources. While browsers still cache them, changing the version number, e.g. after an update, forces the browser to treat the resource as a new one, which is guaranteed to be loaded from the server.

@Kimmax Kimmax changed the title Add version string to scripts / styles to enforce reload after updating Append version string to resources to enforce reload after updating Apr 19, 2018
@fichtner fichtner self-assigned this Aug 23, 2018
@fichtner fichtner added the cleanup Low impact changes label Aug 23, 2018
@fichtner fichtner added this to the 19.1 milestone Aug 23, 2018
@fichtner
Copy link
Member

After much pondering and reports of aggressive browser caching: the plan is to embed the git commit hash into file firmware-product and use this as a version string which is unique and allows us to magically weave this change into the build process.

fichtner added a commit that referenced this issue Sep 2, 2018
Part one makes it possible to inject branding info from the Makefile
which is not fully complete yet but can always be finished.  The new
hash value can thus be used as a unqiue identifier for page resources
that may be subject to caching.  By using the git hash it allows us
to have this effect on test commits as well as earch working version
as we don't want to track the changes for each file but still get a
good amount of caching.
fichtner added a commit that referenced this issue Sep 2, 2018
@fichtner
Copy link
Member

fichtner commented Sep 2, 2018

It's funny, because this has potential for a CVE especially if done on the login page as well as it is a version disclosure. So we'll skip the login page and try to discuss this further internally before shipping it. :)

@fabianfrz
Copy link
Member

@fichtner If you want to do that, the /ui pages need it as well.

@fichtner
Copy link
Member

fichtner commented Sep 2, 2018

no, this is only for resources

@fabianfrz
Copy link
Member

@AdSchellevis
Copy link
Member

We could generate a token after an update to avoid version leakage, the cache_safe function itself would remain the same and is likely the best way to do this.

@fabianfrz this code seems to break ui_devtools at the moment, which looks like your other issue.

@fichtner
Copy link
Member

fichtner commented Sep 2, 2018

I thought commit hash as its more practical than generating a version tag for machine operations, but in any case that inclusion process is settled for both "make install" (and "make upgrade" and all the package building) and could push an arbitrary string into it while doing the operation. The git hash avoids collisions, but the fact remains that the build-time string is not random after shipping it in a release and doing some pseudo-random runtime thingy seems over the top for the problem at hand.

@fabianfrz
Copy link
Member

@fichtner stat can be used: md5(file creation time)

@fichtner
Copy link
Member

fichtner commented Sep 3, 2018

That's still static on build time and you can identify the version from it...

@fabianfrz
Copy link
Member

@fichtner no, it is dynamic (depends on install time). A single nanosecond would give another hash

@fichtner
Copy link
Member

fichtner commented Sep 3, 2018

But then we'd have to write firmware-upgrade or some other arbitrary file or possibly the config.xml which I wanted to avoid in the first place because firmware-upgrade already offers data without overhead.

@fabianfrz
Copy link
Member

no, just call stat on the file which is linked.

opnsense/plugins% echo '<?php $file = "LICENSE"; echo $file . "?" . md5(stat($file)["mtime"]) ?>' | php
LICENSE?eba2cb72eed8a4689ebc202bca9d5146

AdSchellevis added a commit to opnsense/ui_devtools that referenced this issue Sep 3, 2018
@fichtner
Copy link
Member

Ok, this seems ready for general release 4178038 from my side in 18.7.4 after a final testing round in 18.7.3. Some plugins need to be adjusted later though...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Low impact changes
Development

No branches or pull requests

4 participants