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

Add application service support to blueprints #74

Merged
merged 7 commits into from
Feb 23, 2021

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Feb 22, 2021

Add application service support to blueprints

  • Only supports defining a basic app service
  • In tests, you can use the app service as_token to interact with the homserver API
  • Does not support defining the namespaces part of the app service registration config
  • Does not support a mocked/stubbed actual app service that the homeserver calls back to at the URL

Split out from #68


internal/b/blueprints.go Outdated Show resolved Hide resolved
internal/b/blueprints.go Outdated Show resolved Hide resolved
internal/docker/builder.go Outdated Show resolved Hide resolved
internal/docker/builder.go Outdated Show resolved Hide resolved
var buf bytes.Buffer
tw := tar.NewWriter(&buf)
err = tw.WriteHeader(&tar.Header{
Name: fmt.Sprintf("/appservices/%s.yaml", asID),
Copy link
Member

Choose a reason for hiding this comment

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

Please can we do filepath escapes here, as I would like to in the future allow untrusted Blueprint input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume you mean using client.GjsonEscape?

Copy link
Member

Choose a reason for hiding this comment

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

I mean path escaping so you can't have an asID like ../../../etc/passwd

Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

Love it, just a few minor issues to resolve!

@kegsay
Copy link
Member

kegsay commented Feb 23, 2021

LGTM

@kegsay kegsay merged commit 5d63375 into master Feb 23, 2021
@MadLittleMods
Copy link
Contributor Author

Woot! Thanks for the review @kegsay 🐊

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.

2 participants