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

RFC: Don't expose file last modifed date/time via cachebusted URLs #2850

Closed
8 tasks done
roman-yagodin opened this issue Jun 17, 2019 · 10 comments · Fixed by #3531
Closed
8 tasks done

RFC: Don't expose file last modifed date/time via cachebusted URLs #2850

roman-yagodin opened this issue Jun 17, 2019 · 10 comments · Fixed by #3531

Comments

@roman-yagodin
Copy link
Contributor

roman-yagodin commented Jun 17, 2019

Description of problem

Currently raw file last modifed date/time is used to produce cachebusted file URLs, like http://mysite.com/myfile.pdf?ver=2018-04-20-144725-343. I thinks there are cases then we do not want the information about file modification date/time to be exposed in public. Just a sample case - the site owner delayed publishing some important document, but still want it to look just like it is published in time.

Description of solution

Just use some hash string instead of raw file last modifed date/time, something like this: http://mysite.com/myfile.pdf?ver=MjAxOC0wNC0yMC0x.

Affected version

  • 9.3.2
  • 9.3.1
  • 9.2.2
  • 9.2.1
  • 9.2
  • 9.1.1
  • 9.1
  • 9.0
@roman-yagodin
Copy link
Contributor Author

roman-yagodin commented Jun 19, 2019

This will require minor code modification of StandardFolderProvider.GetFileUrl() method and also correcting doc comment for PortalSettings.AddCachebusterToResourceUris(). Don't see any other references to it in the Platform code.

Possible implementations:

1. Use shortened timestamp string (w/o dashes):

public override string GetFileUrl(IFileInfo file)
{
...
    // Does site management want the cachebuster parameter?
    if (portalSettings.AddCachebusterToResourceUris)
    {
        return TestableGlobals.Instance.ResolveUrl(fullPath + "?ver="
            + UrlUtils.EncryptParameter(file.LastModificationTime.ToString("yyyyMMddHHmmssfff")));
    }
...
}

2. Use DateTime.GetHashCode():

Possibly shortest URL.

public override string GetFileUrl(IFileInfo file)
{
...
    // Does site management want the cachebuster parameter?
    if (portalSettings.AddCachebusterToResourceUris)
    {
        return TestableGlobals.Instance.ResolveUrl(fullPath + "?ver="
            + UrlUtils.EncryptParameter(file.LastModificationTime.GetHashCode().ToString()));
    }
...
}

3. Use DateTime.ToBinary() or DateTime.Ticks

Possibly faster to generate.

public override string GetFileUrl(IFileInfo file)
{
...
    // Does site management want the cachebuster parameter?
    if (portalSettings.AddCachebusterToResourceUris)
    {
        return TestableGlobals.Instance.ResolveUrl(fullPath + "?ver="
            + UrlUtils.EncryptParameter(file.LastModificationTime.ToBinary().ToString()));
    }
...
}

@roman-yagodin roman-yagodin changed the title Don't expose file last modifed date/time via cachebusted URLs RFC: Don't expose file last modifed date/time via cachebusted URLs Jun 19, 2019
@sleupold
Copy link
Contributor

sounds useful to me, unless the client would use the current date time to calculate cache time.

@mitchelsellers
Copy link
Contributor

Given that this is a process to bust cache only, and not anything used for calculation we just need to be assured that it is different for all edits.

I'm a big fan of option 2 above, as that is the shortest URL and a simple change.

Feel free to submit a Pull Request for this.

@valadas
Copy link
Contributor

valadas commented Jul 29, 2019

I like the shortest url too.

@valadas
Copy link
Contributor

valadas commented Jul 29, 2019

Assigning this to 10 but if you want it in 9.4.1 and can submit a PR, that would be fine too I thing, that would not be any breaking change I assume?

@valadas valadas added this to the 10.0.0 milestone Jul 29, 2019
@stale
Copy link

stale bot commented Oct 27, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 27, 2019
@roman-yagodin
Copy link
Contributor Author

Please don't close!

@stale stale bot removed the stale label Oct 28, 2019
@stale
Copy link

stale bot commented Jan 26, 2020

We have detected this issue has not had any activity during the last 90 days. That could mean this issue is no longer relevant and/or nobody has found the necessary time to address the issue. We are trying to keep the list of open issues limited to those issues that are relevant to the majority and to close the ones that have become 'stale' (inactive). If no further activity is detected within the next 14 days, the issue will be closed automatically.
If new comments are are posted and/or a solution (pull request) is submitted for review that references this issue, the issue will not be closed. Closed issues can be reopened at any time in the future. Please remember those participating in this open source project are volunteers trying to help others and creating a better DNN Platform for all. Thank you for your continued involvement and contributions!

@stale stale bot added the stale label Jan 26, 2020
@sleupold
Copy link
Contributor

Is anyone planning to provide a PR?

@stale stale bot removed the stale label Jan 26, 2020
@roman-yagodin
Copy link
Contributor Author

It would be me. Please don't close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants