Stop the API accepting a link expiry it discards - #812
Merged
Conversation
Creating a public link over the API accepted `dateExpire` and `maxCountViews`,
advertised both in its own help, and threw both away:
requested: dateExpire=1787003922 maxCountViews=7
stored: dateExpire=1786992179 maxCountViews=3 <- configuration
echoed back: 1787003922, 7 <- never stored
`PublicLink::buildPublicLink()` sets both from configuration on the way to the
database, so neither parameter ever did anything. What made that more than dead
weight is the response, which echoed the request back: a caller asking for a
link that expired at a particular time was told it would. For a feature that
hands an account's password to whoever holds the URL, the expiry is the wrong
thing to be wrong about.
They are removed rather than honoured because `getPublinksMaxTime()` is the
administrator's **maximum**, and a caller who could set the expiry could outlive
it — implementing the documented behaviour would have turned a limit into a
suggestion. Neither web path offers a way to set either; `SaveEditController`
preserves both across an edit.
The response now reports what was stored, by reading the created link back,
which is what `Account\CreateController` and `AuthToken\CreateController`
already do. This endpoint was the one echoing its input.
The test asserts both halves. Only the second — that the answer describes the
link that exists — fails without the fix; the first would pass against the old
code too, since the values were discarded either way.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The API let a caller set a public link's expiry and view limit, discarded both, and confirmed them anyway.
PublicLink::buildPublicLink()sets both from configuration on the way to the database, so the two parameters never did anything. What made that more than dead weight is the response: it echoed the request back, so a caller asking for a link that expired at a particular time was told it would. For a feature that hands an account's password to whoever holds the URL, the expiry is the wrong thing to be wrong about.Why they are removed rather than honoured
getPublinksMaxTime()andgetPublinksMaxViews()are the administrator's configuration, and the first is a maximum. A caller who could set the expiry could outlive it, so implementing the documented behaviour would have turned a limit into a suggestion.Neither web path offers a way to set either —
SaveEditControllerexplicitly preserves both across an edit — so this is policy rather than preference, and the help should not have advertised it.The response now reports what was stored, by reading the created link back. That is the pattern
Account\CreateControllerandAuthToken\CreateControlleralready use; this endpoint was the one that echoed its input instead.Not changed
The response still reports
hash: null, so a caller cannot build the shareable URL without a secondviewcall. Returning the stored model would fix that, but it would also handPublicLink.data— the sealed credential payload — to a token scoped only toPUBLICLINK_CREATE. Worth doing deliberately, with the field list chosen, rather than as a side effect here.Tests
One test, asserting both halves: that the caller's values are not what the row holds, and that the response describes the link that exists. The second is the half that misled anyone, and it is the one that fails when the fix is reverted — the first would pass against the old code too, since the values were discarded either way.
ApiHelpMatchesControllersTestkeeps the two sides honest: removing the parameters from the help without removing the reads, or the reverse, fails it.3965 unit tests pass; PHPStan level 6 on
srcand PHPCS clean.