Skip to content

Allow to configure a config extension per repo#3349

Merged
anbraten merged 85 commits into
woodpecker-ci:mainfrom
anbraten:plugins
Sep 30, 2025
Merged

Allow to configure a config extension per repo#3349
anbraten merged 85 commits into
woodpecker-ci:mainfrom
anbraten:plugins

Conversation

@anbraten
Copy link
Copy Markdown
Member

@anbraten anbraten commented Feb 7, 2024

closes #783

Allow users to set / use service extensions (for this PR just config, secrets / registries could be added later on) on a per repo basis. Those extensions are little external webservice which can implement a set of endpoints to replace woodpeckers internal functionality like:

  • pipeline config processing

TODO

  • add config & secrets extension service urls to repo settings
  • allow to change extensions urls for repo
  • add docs
  • restrict http call locations (prevent local ips)

image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about putting this into an external package? Then we and gitea can use this package.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've created a package / repo, now we only need the Gitea team to use their extracted version. I would move it to the gitea org, it's using my user atm.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@6543 you're still a gitea maintainer, right? Could you check this out?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We can extract it later on if we get a go from Gitea Maintainers.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

uh got a ping - Im out of the loop of this pull atm ... let me reread it

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if it's about that package ... what exact is the request?!?

to let it replace giteas internal one?

Comment thread docs/docs/20-usage/72-extensions/40-configuration-extension.md
Comment thread cmd/server/flags.go

To prevent extensions from calling local services by default only external hosts / ip-addresses are allowed. You can change this behavior by setting the `WOODPECKER_EXTENSIONS_ALLOWED_HOSTS` environment variable. You can use a comma separated list of:

- Built-in networks:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we add none ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What would be the benefit? Disabling it completely?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If that's really needed we could add it in an upcoming PR. As external would not be a security issue IMO and there seems no concrete risk of having the possibility, I would skip it for this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it's the same atack vector as custom agents ... one could create an repo on a public forge+ci and then infite the victim to somehow to create a pull to it. now netrc/token extraction is possible.

we should address this in the long run to generate tokens from forges that are onlv valid for the repo it is currently in use ...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

#3349 (comment)

u might be right ... let me check how oauth2 tokens are used exactly again

anyway having the feat. to derive scope limited tokens for dayli usage and only let wp-server handle the original token would be a good hardening exercize ... is there already an open issue for that?

@6543

This comment was marked as resolved.

@6543

This comment was marked as resolved.

@qwerty287
Copy link
Copy Markdown
Contributor

my attac vector i'm thinking is public instances e.g. codeberg:

Doesn't work.

The netrc data is from the "repo user", which is the user who enabled a repo. It's always this. And never the credentials from the PR opener.

@qwerty287
Copy link
Copy Markdown
Contributor

Besides my point about separating the hostmatcher I actually think this looks good. I didn't test yet, but the code seems fine.

Copy link
Copy Markdown
Contributor

@qwerty287 qwerty287 left a comment

Choose a reason for hiding this comment

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

I'd merge this also if my point is not resolved we can easily do that later as well.

(@6543 can you checkout #3349 (comment))

Comment thread docs/docs/20-usage/72-extensions/index.md
Comment thread docs/docs/20-usage/72-extensions/_category_.yaml Outdated
Comment thread server/services/utils/http.go Outdated
@anbraten anbraten mentioned this pull request Sep 16, 2025
anbraten and others added 3 commits September 22, 2025 10:26
Co-authored-by: qwerty287 <80460567+qwerty287@users.noreply.github.com>
Co-authored-by: qwerty287 <80460567+qwerty287@users.noreply.github.com>
@woodpecker-bot
Copy link
Copy Markdown
Contributor

woodpecker-bot commented Sep 22, 2025

Surge PR preview deployment was removed

Copy link
Copy Markdown
Contributor

@qwerty287 qwerty287 left a comment

Choose a reason for hiding this comment

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

Code lgtm, but untested

Comment thread server/services/utils/hostmatcher/hostmatcher.go
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 29, 2025

Codecov Report

❌ Patch coverage is 50.50000% with 99 lines in your changes missing coverage. Please review.
✅ Project coverage is 19.67%. Comparing base (f819e3c) to head (963e953).
⚠️ Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
server/services/utils/hostmatcher/http.go 0.00% 39 Missing ⚠️
server/services/utils/hostmatcher/hostmatcher.go 80.68% 13 Missing and 4 partials ⚠️
server/services/utils/http.go 64.28% 11 Missing and 4 partials ⚠️
server/model/pagination.go 0.00% 13 Missing ⚠️
server/services/manager.go 0.00% 10 Missing ⚠️
server/api/repo.go 0.00% 3 Missing ⚠️
server/services/setup.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3349      +/-   ##
==========================================
+ Coverage   19.52%   19.67%   +0.14%     
==========================================
  Files         416      419       +3     
  Lines       39567    39782     +215     
==========================================
+ Hits         7726     7827     +101     
- Misses      31144    31249     +105     
- Partials      697      706       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@anbraten anbraten merged commit 677a247 into woodpecker-ci:main Sep 30, 2025
9 checks passed
@woodpecker-bot woodpecker-bot mentioned this pull request Sep 30, 2025
1 task
@qwerty287
Copy link
Copy Markdown
Contributor

I was just wondering: This is breaking right? Because you cannot host a global endpoint on localhost anymore.

@xoxys
Copy link
Copy Markdown
Member

xoxys commented Oct 1, 2025

@anbraten Can you please take a look, this breaks the UI. Can we stop merging such big changes untested?

image

@anbraten
Copy link
Copy Markdown
Member Author

anbraten commented Oct 1, 2025

Will have a look at it. It was tested.

@xoxys
Copy link
Copy Markdown
Member

xoxys commented Oct 1, 2025

I found it already.

@anbraten
Copy link
Copy Markdown
Member Author

anbraten commented Oct 1, 2025

Seems it missing a few changes, maybe they got lost in my merge 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

addon feature add new functionality ui frontend related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Configuration Extension

6 participants