Skip to content

downloader: fix SAS token corruption in constructDatastoreContext#5715

Merged
rene merged 1 commit intolf-edge:masterfrom
jsfakian:Fix-download-image-when-we-include-SAS-token-in-the-url
Apr 11, 2026
Merged

downloader: fix SAS token corruption in constructDatastoreContext#5715
rene merged 1 commit intolf-edge:masterfrom
jsfakian:Fix-download-image-when-we-include-SAS-token-in-the-url

Conversation

@jsfakian
Copy link
Copy Markdown
Contributor

@jsfakian jsfakian commented Mar 30, 2026

Description

url.JoinPath() percent-encodes ? to %3F when it appears inside
a path segment argument. When configName contains a query string
(e.g. an Azure SAS token: ?st=...&se=...&sig=...), the entire query
string is mangled into the path, causing Azure Blob Storage to return
HTTP 409 on the resulting URL, leaving the app instance permanently
stuck in DOWNLOADING state.

Split configName on the first ? before calling url.JoinPath,
apply the path join only to the clean path portion, then reattach the
raw query string after assembly. This preserves SAS tokens and any
other query-string credentials embedded in the relative URL.

The regression was introduced when simple string concatenation was
replaced with url.JoinPath (13.4-stable → 16.7.0). The old
concatenation preserved ? literally; url.JoinPath correctly
encodes it as a path character, breaking datastores configured as HTTP
type with a SAS-bearing relative URL.

How to test and validate this PR

Prerequisites:

  • A device running EVE 16.x
  • A Zededa controller with an application configured to use an HTTP
    datastore pointing to Azure Blob Storage, where the relative URL
    contains a SAS token (i.e., the URL contains ?st=...&sig=...)

Steps to verify the fix:

  1. Apply this patch and build EVE
  2. Run the device with EVE that includes the patch
  3. Onboard the device on a controller with an application configured to use an HTTP
    datastore pointing to Azure Blob Storage, where the relative URL
    contains a SAS token (i.e., the URL contains ?st=...&sig=...)
  4. The download should proceed successfully — no HTTP 409, no %3F in
    the logged URL, CurrentSize increments in the downloader status
  5. The app instance should progress past DOWNLOADING to RUNNING.

Changelog notes

Fixed a regression introduced in 16.0, where application image downloads
from HTTP datastores using Azure SAS tokens (query string credentials
in the relative URL field) failed with HTTP 409, leaving the application
permanently stuck in the DOWNLOADING state. The fix correctly
preserves the SAS query string when constructing the download URL.

PR Backports

  • 16.0-stable: Yes.
  • 14.5-stable: Yes.
  • 13.4-stable: No, same reason as 14.5-stable.

Checklist

  • I've provided a proper description
  • I've added the proper documentation
  • I've tested my PR on amd64 device
  • I've tested my PR on arm64 device
  • I've written the test verification instructions
  • I've set the proper labels to this PR
  • I've checked the boxes above, or I've provided a good reason why I didn't
    check them.

@github-actions github-actions Bot requested a review from eriknordmark March 30, 2026 13:17
@jsfakian jsfakian added the bug Something isn't working label Mar 30, 2026
Copy link
Copy Markdown
Contributor

@rene rene left a comment

Choose a reason for hiding this comment

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

LGTM


// Reattach the raw query string after the path has been assembled.
// We do not re-encode it: the caller is responsible for passing a
// correctly percent-encoded query string in configName.
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.

The caller is responsible for passing a correctly percent-encoded query string in configName.

Is the caller here the controller or the user? In the latter case, we should make sure this requirement is documented.

@eriknordmark
Copy link
Copy Markdown
Contributor

Needs to be rebased on master for the eden tests to work

@jsfakian jsfakian force-pushed the Fix-download-image-when-we-include-SAS-token-in-the-url branch from bcaee89 to 321feb2 Compare April 3, 2026 17:41
@github-actions github-actions Bot requested a review from milan-zededa April 3, 2026 17:41
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 28.34%. Comparing base (2281599) to head (ac8396d).
⚠️ Report is 468 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5715      +/-   ##
==========================================
+ Coverage   19.52%   28.34%   +8.81%     
==========================================
  Files          19       18       -1     
  Lines        3021     2417     -604     
==========================================
+ Hits          590      685      +95     
+ Misses       2310     1588     -722     
- Partials      121      144      +23     

☔ 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.

@jsfakian jsfakian force-pushed the Fix-download-image-when-we-include-SAS-token-in-the-url branch from 321feb2 to aa4b885 Compare April 8, 2026 08:51
@europaul
Copy link
Copy Markdown
Contributor

europaul commented Apr 9, 2026

@jsfakian this bug is coming from my PR #5588, which I also backported to 14.5
so I think the fix should be backported as well

url.JoinPath() percent-encodes '?' to '%3F' when it appears inside
a path segment argument. When configName contains a query string
(e.g. an Azure SAS token: ?st=...&se=...&sig=...), the entire query
string is mangled into the path, causing Azure Blob Storage to return
HTTP 409 on the resulting URL.

Split configName on the first '?' before calling url.JoinPath, apply
the path join only to the clean path portion, then reattach the raw
query string after assembly. This preserves SAS tokens and any other
query-string credentials embedded in the relative URL.

The regression was introduced when simple string concatenation was
replaced with url.JoinPath (13.4-stable -> 16.7.0). The old concat
preserved '?' literally; url.JoinPath correctly encodes it as a path
character, breaking datastores configured with HTTP type and a
SAS-bearing relative URL.

Fixes: app instances stuck in DOWNLOADING state with HTTP 409 from
Azure Blob Storage when datastore is configured as HTTP type with a
SAS token in the relative URL field.

Signed-off-by: Ioannis Sfakianakis <jsfakas@gmail.com>
@rene rene force-pushed the Fix-download-image-when-we-include-SAS-token-in-the-url branch from aa4b885 to ac8396d Compare April 11, 2026 13:34
@rene
Copy link
Copy Markdown
Contributor

rene commented Apr 11, 2026

Updates:

  • Rebased on top of master

@rene rene merged commit 0f17d31 into lf-edge:master Apr 11, 2026
43 of 48 checks passed
@europaul
Copy link
Copy Markdown
Contributor

@jsfakian please take another look at the backporting section - I think this fix needs to be marked as stable and backported
Also see my previous comment #5715 (comment)

@europaul
Copy link
Copy Markdown
Contributor

@jsfakian I backported the fix to 16.0-stable, but 14.5 and 13.4 still need to be done

@jsfakian
Copy link
Copy Markdown
Contributor Author

jsfakian commented Apr 15, 2026

@jsfakian I backported the fix to 16.0-stable, but 14.5 and 13.4 still need to be done

Thank you, @europaul. 13.4 has the old code, so there is no need to backport it there. I can prepare the backport for 14.5

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants