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

feat: setting to override trusted sites list #1819

Merged
merged 4 commits into from
Jan 13, 2025

Conversation

Seb-MCaw
Copy link
Contributor

Closes #814.

The alr publish and alr index --check commands currently require that Git origins use a URL with a host found in a hard-coded list of trusted sites, motivated by concerns regarding SHA1 hash collisions. Those using their own hosting arrangements (with private indexes) need to be able to configure this list.

This PR makes the list configurable, except when using alr publish without the --for-private-index switch.

PR creation checklist
  • A test is included, if required by the changes.
  • doc/user-changes.md has been updated, if applicable.

@Seb-MCaw Seb-MCaw force-pushed the feat/trusted-sites-setting branch from 57a0fa6 to 6f5a74c Compare January 10, 2025 14:18
alire.toml Show resolved Hide resolved
@Seb-MCaw Seb-MCaw marked this pull request as ready for review January 10, 2025 15:09
@Seb-MCaw
Copy link
Contributor Author

Ready for review @mosteo.

Copy link
Member

@mosteo mosteo left a comment

Choose a reason for hiding this comment

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

A minor refactor and a general comment:

I find using ' ' or whitespace in general to mean any domain is a bit hackish and won't be nice when printing settings with alr settings. What do you think about using some impossible domain, like '*' or "..." instead? (Probably the latter to avoid interactions with shell expansions at the time of setting.)

-- Trusted_Sites --
-------------------

function Trusted_Sites (Ignore_Setting : Boolean) return Vector is
Copy link
Member

Choose a reason for hiding this comment

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

Thinking in positive, I'd prefer if Ignore_Setting were For_Community with the same meaning.

@@ -129,7 +129,7 @@ package body Alire.Index is
for Crate of All_Crates.all loop
for Rel of Crate.Releases loop
if Rel.Origin.Kind in Origins.VCS_Kinds then
if not Publish.Is_Trusted (Rel.Origin.URL) then
if not Publish.Is_Trusted (Rel.Origin.URL, False) then
Copy link
Member

Choose a reason for hiding this comment

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

Better use the explicit parameter name with => False

-- 'origins.git.trusted_sites' setting, otherwise it is the hardcoded
-- Community_Trusted_Sites list.

function Is_Trusted (URL : Alire.URL; Ignore_Setting : Boolean)
Copy link
Member

Choose a reason for hiding this comment

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

Likewise about Ignore_Setting, either For_Community or For_Private_Index or even some enum:

type Index_Providers is (Community_Index, Private_Index);

@Seb-MCaw
Copy link
Contributor Author

I find using ' ' or whitespace in general to mean any domain is a bit hackish ...

Agreed. '*' was my first thought, but will definitely cause problems, and an empty list was the best alternative I could come up with. '...' is a good idea, though; thanks for the suggestion.

@Seb-MCaw Seb-MCaw requested a review from mosteo January 13, 2025 11:28
@mosteo mosteo merged commit 09b3893 into alire-project:master Jan 13, 2025
25 checks passed
@mosteo
Copy link
Member

mosteo commented Jan 13, 2025

Merged, thanks.

@Seb-MCaw Seb-MCaw deleted the feat/trusted-sites-setting branch January 13, 2025 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding trusted sites
2 participants