Implement PUT /api/v1/trusted_publishing/github_configs API endpoint
#11113
Conversation
Turbo87
added
C-enhancement ✨
| static RE_VALID_GITHUB_OWNER: LazyLock<regex::Regex> = | ||
| LazyLock::new(|| regex::Regex::new(r"^[a-zA-Z0-9][a-zA-Z0-9-]*$").unwrap()); | ||
|
|
||
| if owner.is_empty() { | ||
| Err(ValidationError::OwnerEmpty) | ||
| } else if owner.len() > MAX_FIELD_LENGTH { | ||
| Err(ValidationError::OwnerTooLong) | ||
| } else if !RE_VALID_GITHUB_OWNER.is_match(owner) { | ||
| Err(ValidationError::OwnerInvalid) |
There was a problem hiding this comment.
This doesn't seem to cover the following cases:
- Ends with
- - Length exceeds
39
| static RE_VALID_GITHUB_REPO: LazyLock<regex::Regex> = | ||
| LazyLock::new(|| regex::Regex::new(r"^[a-zA-Z0-9-_.]+$").unwrap()); | ||
|
|
||
| if repo.is_empty() { | ||
| Err(ValidationError::RepoEmpty) | ||
| } else if repo.len() > MAX_FIELD_LENGTH { | ||
| Err(ValidationError::RepoTooLong) | ||
| } else if !RE_VALID_GITHUB_REPO.is_match(repo) { | ||
| Err(ValidationError::RepoInvalid) |
There was a problem hiding this comment.
This doesn't seem to cover the following cases:
- Reserved names
.and.. - Length exceeds
100
There was a problem hiding this comment.
I think the currently proposed regex might be sufficient, as the regex crate doesn't support lookaround features. If we'd like to harden this part, although there might be some workarounds for lookaround, I think it would be best implemented with simple string-related checks for maintenance reasons.
There was a problem hiding this comment.
I think @eth3lbert stole my thunder on the only actual things I was going to ask about, so otherwise: LGTM. ⚡
|
|
||
| pub fn validate_environment(env: &str) -> Result<(), ValidationError> { | ||
| static RE_INVALID_ENVIRONMENT_CHARS: LazyLock<regex::Regex> = | ||
| LazyLock::new(|| regex::Regex::new(r#"[\x00-\x1F\x7F'"`,;\\]"#).unwrap()); |
There was a problem hiding this comment.
I'm betting you had fun figuring out what "non-printable character" meant in this context (from the GitHub error).
| crates.io user {user} has added a new \"Trusted Publishing\" configuration for GitHub Actions to a crate that you manage ({krate}). Trusted publishers act as trusted users and can publish new versions of the crate automatically. | ||
|
|
||
| Trusted Publishing configuration: | ||
|
|
||
| - Repository owner: {repository_owner} | ||
| - Repository name: {repository_name} | ||
| - Workflow filename: {workflow_filename} | ||
| - Environment: {environment} | ||
|
|
||
| If you did not make this change and you think it was made maliciously, you can remove the configuration from the crate via the \"Settings\" tab on the crate's page. | ||
|
|
||
| If you are unable to revert the change and need to do so, you can email [email protected] to communicate with the crates.io support team." |
There was a problem hiding this comment.
This is fine for now, but we should remember to link this to the documentation we write when this actually gets released.
|
@eth3lbert @LawnGnome probably should've mentioned that in the PR message: the validation rules and email template are more or less taken directly from the PyPI implementation 😅 (@woodruffw please let me know if I need to adjust anything to comply with licensing etc.) I guess technically we don't need the validations at all, since we only need to compare the values with the claims from the OIDC token. They are however useful for:
I don't think we need to be quite as strict as GitHub though, so now I'm wondering why PyPI implemented e.g. the non-printable characters validation 🤔 |
Turbo87
force-pushed
the
trustpub-create-config
branch
from
2d84763 to
2585f04
Compare
This PR implements the first API endpoint for "Trusted Publishing" support in crates.io.
PUT /api/v1/trusted_publishing/github_configscan be used to create Trusted Publishing configurations specific to GitHub Actions. As discussed in #11062 (comment), it seems best to make the configurations provider-specific, and only have the token exchange endpoint be generic.The endpoint is currently limited to cookie authentication, so that we don't have to commit to this API yet, since our frontend will be the only user. Once we have validated that we are happy with the API we can open it up to API token authentication, but that will likely mean introducing another token scope, which seems out of scope for this PR.
This PR is probably best reviewed commit by commit.
Related: