Skip to content

Add cookie support #144

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

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

Add cookie support #144

wants to merge 3 commits into from

Conversation

jkmassel
Copy link
Contributor

@jkmassel jkmassel commented May 29, 2025

What?

Adds the ability to set cookies that'll be used by the editor.

Why?

Ref CMM-235. Close CMM-450. This will make it easy to support private sites for WP.com where the assets require authentication.

How?

Properly routes cookies to where they need to go.

At the moment, there's nothing fancy here – the developer has to know how to properly construct the cookie. We can make this more ergonomic later, if we want.

Testing Instructions

For iOS:

  1. Update ContentView.swift's init method to read:
    init(configuration: EditorConfiguration = .default) {
        var mutableCopy = configuration
        let cookie = HTTPCookie(properties: [
            .domain: "wpmobilep2.wordpress.com",
            .path: "/",
            .name: "wordpress_logged_in",
            .value: "[redacted]",
            .secure: true,
        ])!
        mutableCopy.cookies.append(cookie)
        self.configuration = mutableCopy
    }
  1. Replace [redacted] with the value of the cookie sent when requesting
    https://wpmobilep2.wordpress.com/wp-content/uploads/2025/05/screenshot-2025-05-20-at-3.35.11e280afpm.jpeg from your browser.
  2. Run the demo app and insert the image listed above.
  3. It should show up.
  4. Comment out the cookie injection, try again, note it doesn't work.

For Android:

  1. Update MainActivity's setCookies call to:
            .setCookies(mapOf(
                "https://wpmobilep2.wordpress.com" to "wordpress_logged_in=[redacted]; domain=wpmobilep2.wordpress.com; SameSite=None; Secure; HttpOnly"
            ))
  1. Replace [redacted] with the value of the cookie sent when requesting
    https://wpmobilep2.wordpress.com/wp-content/uploads/2025/05/screenshot-2025-05-20-at-3.35.11e280afpm.jpeg from your browser.
  2. Run the demo app and insert the image listed above.
  3. It should show up.
  4. Comment out the cookie injection, try again, note it doesn't work.

@jkmassel jkmassel requested a review from dcalhoun May 29, 2025 22:27
@jkmassel jkmassel self-assigned this May 29, 2025
@jkmassel jkmassel added the [Type] Enhancement A suggestion for improvement. label May 29, 2025
Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

The simplicity of the cookie approach feels so much more sound than a proxy. Nicely done.

I performed the testing instructions with private image, video, and audio files. All functioned as expected on both iOS and Android.

The one media that failed was VideoPress URLs. First, it appears VideoPress relies upon a URL token parameter for authentication and the cookie has no impact. Second, the editor seemed to fail handling 206 responses when token authentication succeeded. That said, neither of these seem related to the proposed cookie authentication.

@dcalhoun
Copy link
Member

Noting this replaces #30 and supersedes #33, which can be removed and closed respectively.

Also, we can close wordpress-mobile/WordPress-Android#21331 as its approach is no longer applicable.

We will need to implement appropriate cookie configuration in WP-iOS and WP-Android after merging this work.

@jkmassel
Copy link
Contributor Author

jkmassel commented Jun 9, 2025

@dcalhoun I'll leave this to you to merge given the breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants