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

@uppy/companion: make CSRF protection helpers available to providers #4554

Merged
merged 6 commits into from
Aug 7, 2023

Conversation

dschmidt
Copy link
Contributor

@dschmidt dschmidt commented Jul 6, 2023

see #4551 for context

@dschmidt dschmidt force-pushed the reusable-helpers branch 2 times, most recently from e27ce50 to 72e7443 Compare July 9, 2023 12:15
@aduh95 aduh95 requested a review from mifi July 13, 2023 13:53
Copy link
Contributor

@mifi mifi left a comment

Choose a reason for hiding this comment

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

exposing the helper lgtm, but i don't think the other changes belong in this PR?

Comment on lines 7 to 13
* @param {object} options
* @param {{providerName: string, allowLocalUrls: boolean, providerOptions: object}} options
*/
constructor (options) { // eslint-disable-line no-unused-vars
constructor ({ allowLocalUrls, providerOptions }) {
// Some providers might need cookie auth for the thumbnails fetched via companion
this.needsCookieAuth = false
this.allowLocalUrls = allowLocalUrls
this.providerOptions = providerOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these changes belong in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

providerOptions probably is not necessary at this point
allowLocalUrls is injected to the provider so we know what to pass to validateUrl, so I'd say: yes, this belongs into this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

but isn't this PR about simply making the helpers validateURL and getProtectedHttpAgent plublically exposed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, yes, but I need to be able to use it? Without introducing my own env var handling in my provider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the providerOptions (to send them in another PR at some point) but I stick to it: it's a good idea and it belongs into this PR to make the global allowLocalUrls option available to the provider.

Comment on lines 56 to 58
req.companion.provider = new ProviderClass({ providerName })
const providerOptions = req.companion.options.providerOptions[providerName] || {}
const { allowLocalUrls } = req.companion.options
req.companion.provider = new ProviderClass({ providerName, providerOptions, allowLocalUrls })
Copy link
Contributor

Choose a reason for hiding this comment

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

some with these?

This makes it possible to create protected http agents in providers and pass them on to networking libraries.
@Murderlon Murderlon requested a review from mifi August 1, 2023 08:27
@Murderlon Murderlon merged commit 22583dc into transloadit:main Aug 7, 2023
18 checks passed
github-actions bot added a commit that referenced this pull request Aug 15, 2023
| Package                   | Version | Package                   | Version |
| ------------------------- | ------- | ------------------------- | ------- |
| @uppy/audio               |   1.1.2 | @uppy/locales             |   3.3.0 |
| @uppy/aws-s3              |   3.2.2 | @uppy/onedrive            |   3.1.3 |
| @uppy/aws-s3-multipart    |   3.5.3 | @uppy/progress-bar        |   3.0.3 |
| @uppy/box                 |   2.1.3 | @uppy/provider-views      |   3.5.0 |
| @uppy/companion           |   4.8.0 | @uppy/redux-dev-tools     |   3.0.3 |
| @uppy/companion-client    |   3.3.0 | @uppy/screen-capture      |   3.1.2 |
| @uppy/core                |   3.4.0 | @uppy/status-bar          |   3.2.4 |
| @uppy/dashboard           |   3.5.1 | @uppy/thumbnail-generator |   3.0.4 |
| @uppy/drag-drop           |   3.0.3 | @uppy/transloadit         |   3.2.1 |
| @uppy/dropbox             |   3.1.3 | @uppy/tus                 |   3.1.3 |
| @uppy/facebook            |   3.1.2 | @uppy/unsplash            |   3.2.2 |
| @uppy/file-input          |   3.0.3 | @uppy/url                 |   3.3.3 |
| @uppy/google-drive        |   3.2.1 | @uppy/webcam              |   3.3.2 |
| @uppy/image-editor        |   2.1.3 | @uppy/xhr-upload          |   3.3.2 |
| @uppy/informer            |   3.0.3 | @uppy/zoom                |   2.1.2 |
| @uppy/instagram           |   3.1.2 | uppy                      |  3.14.0 |

- meta: Readme improvements (Artur Paikin / #4622)
- @uppy/companion: Fix typos and add env vars to .env.example (Dominik Schmidt / #4624)
- @uppy/aws-s3-multipart: pass the `uploadURL` back to the caller (Antoine du Hamel / #4614)
- meta: update to node-18.17.0-alpine,  (odselsevier / #4617)
- @uppy/aws-s3,@uppy/aws-s3-multipart: update types (Antoine du Hamel / #4611)
- @uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/companion,@uppy/transloadit,@uppy/xhr-upload: use uppercase HTTP method names (Antoine du Hamel / #4612)
- meta: e2e: fix race condition in transloadit test (Antoine du Hamel / #4616)
- @uppy/aws-s3,@uppy/aws-s3-multipart: update types (bdirito / #4576)
- @uppy/core: allow duplicate files with onBeforeFileAdded (Merlijn Vos / #4594)
- @uppy/companion: make CSRF protection helpers available to providers (Dominik Schmidt / #4554)
- @uppy/companion: fix Redis key default TTL (Subha Sarkar / #4607)
- @uppy/companion: Fix Uploader.js metadata normalisation (Subha Sarkar / #4608)
- @uppy/companion-client,@uppy/provider-views: make authentication optional (Dominik Schmidt / #4556)
- @uppy/provider-views: fix ProviderView error on empty plugin.icon (Dominik Schmidt / #4553)
- @uppy/aws-s3,@uppy/tus,@uppy/xhr-upload:  Invoke headers function for remote uploads (Dominik Schmidt / #4596)
- @uppy/companion: Unify redis initialization (Dominik Schmidt / #4597)
- meta: lock node-js version on ci (Mikael Finstad / #4606)
- @uppy/companion: allow dynamic S3 bucket (rmoura-92 / #4579)
- @uppy/status-bar: e2e: add test for retrying and pausing uploads (Antoine du Hamel / #3599)
- meta: e2e: remove too short timeout (Antoine du Hamel / #4602)
Murderlon added a commit that referenced this pull request Aug 24, 2023
* main: (28 commits)
  Release: [email protected] (#4644)
  @uppy/aws-s3-multipart: fix types when using deprecated option (#4634)
  @uppy/companion: harden lint rules (#4641)
  allow empty objects for `fields` types (#4631)
  meta: upgrade Node.js docker version (#4630)
  Release: [email protected] (#4626)
  Readme improvements (#4622)
  Fix typos and add env vars to .env.example (#4624)
  @uppy/aws-s3-multipart: pass the `uploadURL` back to the caller (#4614)
  update to node-18.17.0-alpine,  (#4617)
  @uppy/aws-s3,@uppy/aws-s3-multipart: update types (#4611)
  use uppercase HTTP method names (#4612)
  e2e: fix race condition in transloadit test (#4616)
  @uppy/aws-s3,@uppy/aws-s3-multipart: update types (#4576)
  @uppy/core: allow duplicate files with onBeforeFileAdded (#4594)
  @uppy/companion: make CSRF protection helpers available to providers (#4554)
  @uppy/companion: fix Redis key default TTL (#4607)
  @uppy/companion: Fix Uploader.js metadata normalisation (#4608)
  @uppy/companion-client,@uppy/provider-views: make authentication optional (#4556)
  @uppy/provider-views: fix ProviderView error on empty plugin.icon (#4553)
  ...
@dschmidt dschmidt deleted the reusable-helpers branch June 13, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants