chore(plugin-server): Validate fetch hostnames
#17183
Conversation
plugin-server/src/utils/fetch.ts
Outdated
| if (ipaddr.parse(address).range() !== 'unicast') { | ||
| throw new FetchError('Invalid hostname', 'posthog-host-guard') |
There was a problem hiding this comment.
Not sure if it makes any sense to also support ranges such as mulitcast, Teredo, or 6to4? @ellie
There was a problem hiding this comment.
I think it would make sense, but can be done in a follow up
Twixes
force-pushed
the
validate-app-fetch
branch
5 times, most recently
from
72226ef to
be6fbaa
Compare
Twixes
force-pushed
the
validate-app-fetch
branch
from
be6fbaa to
a030f1f
Compare
| @@ -399,8 +404,10 @@ export class HookCommander { | |||
| `⌛⌛⌛ Posting RestHook slow. Timeout warning after 5 sec! url=${hook.target} team_id=${event.teamId} event_id=${event.eventUuid}` | |||
| ) | |||
| }, 5000) | |||
| const relevantFetch = | |||
| isCloud() && this.fetchHostnameGuardTeams.has(hook.team_id) ? safeTrackedFetch : trackedFetch | |||
There was a problem hiding this comment.
just calling out this line to any other reviewers as being the "opt in"
|
Yup @ellie merging this should have zero effect until the |
| @@ -264,11 +266,13 @@ export class HookCommander { | |||
| postgres: PostgresRouter, | |||
| teamManager: TeamManager, | |||
| organizationManager: OrganizationManager, | |||
| fetchHostnameGuardTeams: Set<number>, | |||
There was a problem hiding this comment.
since we're going to remove it in the future and have all teams opt into this I'd add it as the last arg and optional, so there's less test changes needed when removing
There was a problem hiding this comment.
Makes sense, made optional – although didn't put it last, because we never pass statsd in in these tests, while we do pass this
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
Twixes
force-pushed
the
validate-app-fetch
branch
from
261e5f7 to
23db257
Compare
Twixes
force-pushed
the
validate-app-fetch
branch
from
23db257 to
02c6e0e
Compare
Twixes
force-pushed
the
validate-app-fetch
branch
from
7fff39c to
04eca49
Compare
Changes
The plugin server equivalent of #17147. Note that this introduces some performance overhead in the form of DNS resolution, so we'll need to monitor fetch timing metrics after deploying this.