Skip to content

bound array access in StoragePath url parsers #16243

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.

Sign up for GitHub

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

Open
alhudz wants to merge 3 commits into
base: main
Choose a base branch
from
+36 −2
Open

bound array access in StoragePath url parsers #16243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7db8a3b
bound array access in StoragePath url parsers
alhudz Jun 9, 2026
77ffe02
validate /o marker and simplify object lookup in StoragePath parsers
alhudz Jun 16, 2026
d5f6624
validate /o marker before the object-path guard in HTTP parser
alhudz Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions FirebaseStorage/Sources/Internal/StoragePath.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,10 @@ class StoragePath: NSCopying, Equatable {
let bucketObject = aURIString.dropFirst("gs://".count)
if bucketObject.contains("/") {
let splitStringArray = bucketObject.split(separator: "/", maxSplits: 1).map(String.init)
let object = splitStringArray.count == 2 ? splitStringArray[1] : nil
return StoragePath(with: splitStringArray[0], object: object)
if let bucketName = splitStringArray.first {
let object = splitStringArray.dropFirst().first
return StoragePath(with: bucketName, object: object)
}
Comment on lines +61 to +64

@gemini-code-assist gemini-code-assist Bot Jun 12, 2026

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.

low

Instead of checking splitStringArray.count == 2 and accessing the index splitStringArray[1], you can use splitStringArray.dropFirst().first to safely and idiomatically retrieve the optional object path. This is cleaner and avoids hardcoded index checks.

Suggested change
if let bucketName = splitStringArray.first {
let object = splitStringArray.count == 2 ? splitStringArray[1] : nil
return StoragePath(with: bucketName, object: object)
}
if let bucketName = splitStringArray.first {
let object = splitStringArray.dropFirst().first
return StoragePath(with: bucketName, object: object)
}

@alhudz alhudz Jun 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done, switched to splitStringArray.dropFirst().first. Drops the count == 2 check and the hardcoded [1] index.

} else if bucketObject.count > 0 {
return StoragePath(with: String(bucketObject))
}
Expand Down Expand Up @@ -91,6 +93,13 @@ class StoragePath: NSCopying, Equatable {
guard pathComponents.count > 4 else {
return StoragePath(with: bucketName)
}
guard pathComponents[4] == "o" else {
throw StoragePathError.storagePathError("Internal error: URL must be in the form of " +
"http[s]://<host>/v0/b/<bucket>/o/<path/to/object>[?token=signed_url_params]")
}
guard pathComponents.count > 5 else {
return StoragePath(with: bucketName)
}
// Construct object name
var objectName = pathComponents[5]
for i in 6 ..< pathComponents.count {
Expand Down
25 changes: 25 additions & 0 deletions FirebaseStorage/Tests/Unit/StoragePathTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,14 +84,39 @@ class StoragePathTests: XCTestCase {
XCTAssertEqual(path.object, "#hashtag/no/token")
}

func testHTTPURLObjectMarkerWithoutObject() throws {
// "/o" object marker present but no object path; must not index past the
// path components.
let httpURL = "http://firebasestorage.googleapis.com/v0/b/bucket/o"
let path = try StoragePath.path(string: httpURL)
XCTAssertEqual(path.bucket, "bucket")
XCTAssertNil(path.object)
}

func testGSURIThrowsOnNoBucket() {
XCTAssertThrowsError(try StoragePath.path(string: "gs://"))
}

func testGSURIThrowsOnOnlySlashes() {
XCTAssertThrowsError(try StoragePath.path(string: "gs:///"))
}

func testHTTPURLThrowsOnNoBucket() {
XCTAssertThrowsError(try StoragePath.path(string: "http://firebasestorage.googleapis.com/"))
}

func testHTTPURLThrowsOnMissingObjectMarker() {
// A path segment in the "/o" marker position that isn't "o" is not a valid
// Storage URL and must throw rather than be parsed as an object.
let httpURL = "http://firebasestorage.googleapis.com/v0/b/bucket/x/path/to/object"
XCTAssertThrowsError(try StoragePath.path(string: httpURL))

// Same invalid marker but with no object path (exactly five components) must
// also throw rather than be parsed as a bucket-only path.
let shortHTTPURL = "http://firebasestorage.googleapis.com/v0/b/bucket/x"
XCTAssertThrowsError(try StoragePath.path(string: shortHTTPURL))
}
Comment on lines +108 to +118

@gemini-code-assist gemini-code-assist Bot Jun 16, 2026

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.

low

To prevent regressions, it would be beneficial to also test the case where the URL has exactly 5 path components but the 5th component is not "o" (e.g., http://firebasestorage.googleapis.com/v0/b/bucket/x).

  func testHTTPURLThrowsOnMissingObjectMarker() {
    // A path segment in the "/o" marker position that isn't "o" is not a valid
    // Storage URL and must throw rather than be parsed as an object.
    let httpURL = "http://firebasestorage.googleapis.com/v0/b/bucket/x/path/to/object"
    XCTAssertThrowsError(try StoragePath.path(string: httpURL))

    let shortHttpURL = "http://firebasestorage.googleapis.com/v0/b/bucket/x"
    XCTAssertThrowsError(try StoragePath.path(string: shortHttpURL))
  }

@alhudz alhudz Jun 29, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added the five-component case (.../bucket/x) to testHTTPURLThrowsOnMissingObjectMarker alongside the existing longer one, so both assert a throw now.


func testThrowsOnInvalidScheme() {
let ftpURL = "ftp://firebasestorage.googleapis.com/v0/b/bucket/o/path/to/object"
XCTAssertThrowsError(try StoragePath.path(string: ftpURL))
Expand Down
Loading