Skip to content

Allow setting the HTTP Origin header - #61

Open
OlenaPostindustria wants to merge 5 commits into
masterfrom
59/allow-setting-origin-header
Open

Allow setting the HTTP Origin header#61
OlenaPostindustria wants to merge 5 commits into
masterfrom
59/allow-setting-origin-header

Conversation

@OlenaPostindustria

Copy link
Copy Markdown
Collaborator

Closes #59

@eugenedorfman-optable eugenedorfman-optable left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Two things to change, plus a doc nit.

Blank origin sends an empty header. Source/Core/EdgeAPI.swift:157 sets the header from whatever is configured, so config.origin = "" produces a literal Origin: with an empty value. The Android SDK suppresses blank values (isNullOrBlank() in RequestInterceptor.kt) and has tests for both blank and null, so the two SDKs disagree here. Suggested fix:

// NOTE: A blank origin would send an empty `Origin:` header, which is worse than sending none.
// Android suppresses blank values too, so both SDKs behave the same.
if let origin = config.origin, !origin.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
    headers[.origin] = origin
}

with a test covering "" and " ", and the doc comment on OptableConfig.origin updated to say a blank value is treated as unset.

Unrelated dependency churn. demo-ios-objc/Podfile.lock:2-11 bumps Google-Mobile-Ads-SDK 12.14.0 -> 13.7.0 and PrebidMobile 3.1.0 -> 3.3.2. That looks like a side effect of running pod install against the unpinned Podfile rather than anything this change needs. Please revert those hunks; if a demo dependency refresh is wanted, it belongs in its own PR.

Doc example value. docs/usage-swift.md:57 and docs/usage-objc.md:81 use https://dcn.customer.com, which these same docs use as the DCN endpoint hostname. The header is meant to carry the site origin that traffic is attributed to, so a customer-site example reads better: the Android README uses https://www.acmeco.com along with the wording "the origin you want your mobile traffic attributed to". Adding Android's "Unrelated to originSlug" note to the origin doc comment would help too, since the two names are easy to confuse.

On the cross-SDK side, for reference: origin is mutable after construction here, but was an internal val on Android, which left Java callers reaching it only through the full 11-argument constructor. The Android PR has been updated to make it a settable var, matching this SDK.

@eugenedorfman-optable eugenedorfman-optable left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The three points from the earlier review are addressed: blank and whitespace-only origins are suppressed and covered by tests, the Podfile.lock dependency bumps are reverted so the diff against master no longer touches them, and the docs now use a site-origin example with the attribution wording plus the note that this is unrelated to originSlug. CI is green.

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.

Allow setting the HTTP Origin header

2 participants