sysutils/git-backup: don't accept credentials in the backup url - #5702
Conversation
|
It's probably better to improve the field validation as we're looking at an input error here (I don't think usernames inside uri's where ever supported here, in which case we can just change the regex to exclude the user section) |
|
Thanks — that's fair. The field help and the validation message both show user-less examples, so a username in the URL was never intended input here. One consequence worth weighing before I rework it: validation only runs on save. Measured on 26.1.11_10 against a real SSH remote, same stored
The last two rows differ only in So it comes down to whether those setups should recover on their own or be asked to fix their input. Your call, and I am happy either way. If validation only, this one came back clean on the ten inputs I ran through <Mask>/^((https)|(ssh))?:\/\/[^\/@]+(\/.*)?[^\/]$/</Mask>paired with a validation message pointing at the User Name field. The Let me know which you would prefer and I will rewrite the PR. |
|
Usually if saved data should be changed due to model changes, we use migrations to update the data, but only when the data should be altered due to a change in how the model treats the data underneath (e.g. we split one input into two). In this case there's just an omission in the validation allowing more than it should, which would plea for just repairing the validation. These things are quite common, we tighten validations frequently usually because the input or combinations of inputs aren't actually supported by the technology underneath. |
The url mask allows a userinfo section, and Git::backup() adds the mandatory User Name field on top, so an ssh url that already carries a user ends up as ssh://git@git@host/... and the push never authenticates. Credentials belong in the User Name and Password fields, so exclude them from the url. Closes opnsense#5477 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e0fef91 to
a3cb83e
Compare
|
Rewritten as a validation change only: One correction I owe you from my earlier comment. I wrote that already-configured installations would be told the next time they saved that page. That is not right, and I should have measured it before saying it. I also checked that neither credential-bearing form works today, over ssh and over https, so no setup that currently backs up successfully becomes invalid. |
|
@Lux-WorldPC looks good, thanks! |
Important notices
Before you submit a pull request, we ask you kindly to acknowledge the following:
If AI was used, please disclose:
a run through the web form on a real installation, and I reviewed each one before posting.
Describe the problem
The
urlmask allows a userinfo section, so a url that already carries a user passes validation.Git::backup()then adds the User Name field on top — mandatory once the backup is enabled — andthe remote ends up with the user twice:
The push never authenticates and nothing is backed up. Reported in #5477 and #5606.
Describe the proposed solution
As you suggested, repair the validation rather than the code: exclude the userinfo part of the
authority from the mask, and say so in the validation message. Credentials have their own User Name
and Password fields, and the field help only ever showed user-less examples.
Git.phpis untouched.The exclusion covers the authority only, so an
@stays legal inside the path.Tested on 26.1.11_10 with os-git-backup 1.1_3, through the form on
diag_backup.php, against anssh remote and an nginx/
git-http-backendremote. Of nineteen urls, four change — the two from thereports plus
https://user@host/repo.gitandhttps://user:token@host/repo.git. Ports, an IPv6literal and an
@inside the path stay accepted;git@github.com:user/project.gitstays rejected.Both intended forms still save and push, over ssh and over https. Credentials in the authority are
the only thing that changes: an empty authority and an empty scheme are still accepted, exactly as
before.
Neither credential-bearing form works today, so nothing that currently backs up becomes invalid:
with such a url stored directly in
config.xml, the remote comes out asssh://git@git@host/...and
https://git:pw@git:pw@host/..., and no commit reaches the server.One limitation worth stating: this does not reach installations that already stored such a url.
performValidation()only validates fields that changed (BaseModel.php:637), so re-saving thatpage with the url untouched passes the new mask — I ran it, and changing only the branch too; both
report
authentication failurewith nothing pointing at the url. The new message appears only oncethat field is edited. No migration, as you said: the model's treatment of the data is unchanged.
PLUGIN_REVISIONleft alone.Related issue
Closes #5477
Refs #5606, #5629