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

HTTP date format fixes #984

Merged
merged 1 commit into from
May 12, 2020
Merged

HTTP date format fixes #984

merged 1 commit into from
May 12, 2020

Conversation

osma
Copy link
Member

@osma osma commented May 11, 2020

While testing HTTP 304 / If-Modified-Since functionality, recently implemented in PR #869 by @kinow, I noticed that the date formats expected by Skosmos in If-Modified-Since headers and produced in Last-Modified headers are not actually compliant with the relevant RFCs (RFC 7231 / 2616 / 1123). I also noticed other problems related to that functionality. This PR fixes:

  • Read the If-Modified-Since header using the filter_input function instead of $_SERVER (possibly more secure, and in line with other places in the codebase where HTTP headers are accessed)
  • Change to RFC 1123 date format for HTTP 304 functionality
  • Improve the Http304Test unit tests so that they verify that the mocked methods are actually called exactly once with the expected arguments (otherwise they couldn't detect invalid date formats)

Any comments @kinow before I merge this?

@osma osma added the bug label May 11, 2020
@osma osma added this to the 2.6 milestone May 11, 2020
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov
Copy link

codecov bot commented May 11, 2020

Codecov Report

Merging #984 into master will increase coverage by 0.00%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #984   +/-   ##
=========================================
  Coverage     59.11%   59.12%           
  Complexity     1548     1548           
=========================================
  Files            32       32           
  Lines          4334     4335    +1     
=========================================
+ Hits           2562     2563    +1     
  Misses         1772     1772           
Impacted Files Coverage Δ Complexity Δ
controller/Controller.php 40.15% <75.00%> (+0.45%) 54.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f28514...c24af1b. Read the comment docs.

Copy link
Collaborator

@kinow kinow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!! Thanks @osma!

@osma
Copy link
Member Author

osma commented May 12, 2020

Verified with REDbot.org.

Current situation (before the merge):

Last-Modified: 2020-05-12 05:36:10

General
The Last-Modified header's value isn't a valid date.

@osma osma merged commit 80fad12 into master May 12, 2020
@kinow
Copy link
Collaborator

kinow commented May 12, 2020

Verified with REDbot.org.

Bookmarked! Great tool, thanks!

@osma
Copy link
Member Author

osma commented May 12, 2020

After the merge:

Last-Modified: Tue, 12 May 2020 06:01:45 GMT

If-Modified-Since conditional requests are supported.

(and the error is gone)

Bookmarked! Great tool, thanks!

Yes, it's good - but the CAPTCHAs are tiring. They didn't use them before but probably there was some abuse they needed to counter

@kinow
Copy link
Collaborator

kinow commented May 12, 2020

Yes, it's good - but the CAPTCHAs are tiring. They didn't use them before but probably there was some abuse they needed to counter

I'd like to enable it for the build of some tools I am working on. They have the source code available https://github.com/mnot/redbot, and even provide a Docker image.

In theory it should be possible to spin up a Docker instance with the latest SKOSMOS deployed, and another container with REDbot.

And then just use curl or something to query the REDbot container giving the URL to the SKOSMOS container 🤔

@osma
Copy link
Member Author

osma commented May 12, 2020

I'd like to enable it for the build of some tools I am working on.

Hmm - in my understanding it's an interactive debugging tool, not something to automate. But maybe I've missed some aspect?

@osma osma deleted the fix-modified-date-format branch May 12, 2020 06:50
@kinow
Copy link
Collaborator

kinow commented May 12, 2020

Hmm - in my understanding it's an interactive debugging tool, not something to automate. But maybe I've missed some aspect?

No, I think you are correct.

But assuming it's possible to create a curl call to send an HTTP request to a locally deployed REDbot instance, then I think it means it would be possible to also automate it in the build process 🤓

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

Successfully merging this pull request may close these issues.

2 participants