File.publicUrl() returns an incorrect URL if the file key contains URL query parameters #1824

Activity

1. jbrowning

   added

   priority: p2Moderately-important priority. Fix may not be included in next release.Moderately-important priority. Fix may not be included in next release.

   type: bugError or flaw in code with unintended results or allowing sub-optimal usage patterns.Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

   on Mar 24, 2022
2. product-auto-label

   added

   api: storageIssues related to the googleapis/nodejs-storage API.Issues related to the googleapis/nodejs-storage API.

   on Mar 24, 2022
3. ddelgrosso1 commented on Mar 28, 2022

   

   ddelgrosso1

   on Mar 28, 2022

   Contributor

   More actions

   Thank you for bringing this to our attention @jbrowning. I have identified what I believe to be the source of the problem you are encountering and will be working on a fix. I will update this issue soon with any further findings / when a fix is available.
4. ddelgrosso1

   self-assigned this

   on Mar 28, 2022
5. shaffeeullah

   closed this as completedin #1828on Mar 28, 2022
6. ddelgrosso1 commented on Mar 28, 2022

   

   ddelgrosso1

   on Mar 28, 2022

   Contributor

   More actions

   @jbrowning what I believe to be the fix for this has been released in version 5.18.3.
7. jbrowning commented on Mar 28, 2022

   

   jbrowning

   on Mar 28, 2022

   Author

   More actions

   @ddelgrosso1 Perfect. Thanks for the quick fix.
8. sseppola commented on Apr 7, 2022

   

   sseppola

   on Apr 7, 2022

   Last edited by sseppola

   More actions

   @ddelgrosso1 was it intentional that filenames with paths now turn from upload/video/test.mp4 to upload%2Fvideo%2Ftest.mp4?
   It still works, and the GCS UI shows the directories as expected, but the public urls I store in my DB are less readable.

   Edit:
   Would you consider something like:

   publicUrl(): string {
   	const url = new URL(`${this.storage.apiEndpoint}/${this.bucket.name}/${this.name}`);
   	return url.origin + url.pathname + encodeURIComponent(url.search)
   }

   This should be Nodejs 10 compatible according to: https://developer.mozilla.org/en-US/docs/Web/API/URL
9. ddelgrosso1 commented on Apr 7, 2022

   

   ddelgrosso1

   on Apr 7, 2022

   Contributor

   More actions

   Hi @sseppola it was intentional from the standpoint that other spots in the code are currently encoding the entire name portion as I applied in the fix. Your suggestion makes sense to me, let me investigate if switching to something like this will have any other possible consequences.

   👍React with 👍1Reacted by Sindre Seppola
10. ddelgrosso1 commented on Apr 7, 2022

    

    ddelgrosso1

    on Apr 7, 2022

    Contributor

    More actions

    @sseppola I should have realized this before but the reason it was done this way is due to the object naming guidelines in GCS. In the above snippet this.name represents the object name, which can support characters such as spaces, ampersands, etc. In order to return a properly working public url the entire name will need to be encoded to account for these cases.
11. sseppola commented on Apr 7, 2022

    

    sseppola

    on Apr 7, 2022

    More actions

    Makes sense, thanks for resolving it so quickly 👍