Skip to content

fix: prefer proxy password from env var #385

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

Merged
stepskop merged 7 commits into from
May 7, 2025
Merged

fix: prefer proxy password from env var #385

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
497f0c5
chore: fetch proxy password only if env not provided
stepskop May 5, 2025
a057e5a
chore: return early
stepskop May 5, 2025
725bf4c
chore: remove skipped test
stepskop May 5, 2025
24b1b36
refactor: log from init
stepskop May 6, 2025
be9f532
fix: typo
stepskop May 6, 2025
d0e41d4
chore: simplify and docs
stepskop May 6, 2025
288f8e0
docs: add space
stepskop May 6, 2025
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
18 changes: 6 additions & 12 deletions packages/apify/src/proxy_configuration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,7 @@ export class ProxyConfiguration extends CoreProxyConfiguration {
* Checks if Apify Token is provided in env and gets the password via API and sets it to env
*/
protected async _setPasswordIfToken(): Promise<void> {
Copy link

@tobice tobice May 6, 2025

Choose a reason for hiding this comment

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

I don't think the method name and the documentation make sense anymore.

The logic should be as follows:

  • If the password is provided through env, use that.
  • If not, try to use the token to fetch the password from API as fallback.

Considering the existing implementation, I would:

  1. Rename this to something like fetchPasswordFromApi.
  2. Update the initialize function to use this method.
    async initialize(): Promise<boolean> {
        if (this.usesApifyProxy) {
            if (!this.password) {
                 this.password = await this.fetchPasswordFromApi();
            }
            
            if (!this.password) {
                 // Do the logging here.
            }
            
            return this._checkAccess();
        }
        return true;

IMHO this will capture way better what the logic is.

Copy link
Member

@B4nan B4nan May 6, 2025

Choose a reason for hiding this comment

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

For some reason, the method is protected, so technically, renaming it is a BC.

(I guess it should be safe, but still.)

Copy link

@tobice tobice May 6, 2025

Choose a reason for hiding this comment

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

Ah, that's true... well, it's kind of your call here 😄

We could also keep it around, mark it as deprecated, and from within do this.password = fetchPasswordFromApi().

Copy link
Member

@B4nan B4nan May 6, 2025

Choose a reason for hiding this comment

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

I would rather stay on the safe side and keep it around, marked as deprecated or have a TODO on it so we can rename and make it private in the next major (which we will hopefully get to later this year or early next year).

Copy link
Contributor Author

@stepskop stepskop May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We can keep the name and make the checks and warnings as @tobice proposed:

    async initialize(): Promise<boolean> {
        if (this.usesApifyProxy) {
            if (!this.password) {
                 await this._setPasswordIfToken();
            }
            
            if (!this.password) {
                 // Do the logging here.
            }
            
            return this._checkAccess();
        }
        return true;

That makes the most sense to me.

Edit: ...and adding TODO

Copy link

@tobice tobice May 6, 2025

Choose a reason for hiding this comment

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

Looks good 👍

if (this.password) return;
const token = this.config.get('token');

if (token) {
Expand All @@ -440,29 +441,22 @@ export class ProxyConfiguration extends CoreProxyConfiguration {

const { password } = proxy!;

if (this.password) {
if (this.password !== password) {
this.log.warning(
'The Apify Proxy password you provided belongs to' +
' a different user than the Apify token you are using. Are you sure this is correct?',
);
}
} else {
this.password = password;
}
this.password = password;
Copy link

@tobice tobice May 6, 2025

Choose a reason for hiding this comment

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

(nit) IMHO this whole function can be now simplified.

    protected async _setPasswordIfToken(): Promise<void> {
        const token = this.config.get('token');
        
        if (!token) {
            return;
        }
        
        try {
            const user = await Actor.apifyClient.user().get();
            this.password = user.proxy?.password;
        } catch (error) {
            if (Actor.isAtHome()) {
                throw error;
            } else {
                this.log.warning(`Failed to fetch user data using token`,  { error });
            }
        }
    }

Also disabling proxy is misleading within the scope of this function.

Copy link
Contributor Author

@stepskop stepskop May 6, 2025

Choose a reason for hiding this comment

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

Done.

}

if (!this.password) {
if (Actor.isAtHome()) {
throw new Error(
`Apify Proxy password must be provided using options.password or the "${APIFY_ENV_VARS.PROXY_PASSWORD}" environment variable. ` +
`If you add the "${APIFY_ENV_VARS.TOKEN}" environment variable, the password will be automatically inferred.`,
`If you add the "${APIFY_ENV_VARS.TOKEN}" environment variable, ` +
`while not providing "${APIFY_ENV_VARS.PROXY_PASSWORD}", the password will be automatically inferred.`,
);
} else {
this.log.warning(
`No proxy password or token detected, running without proxy. To use Apify Proxy locally, ` +
`provide options.password or "${APIFY_ENV_VARS.PROXY_PASSWORD}" environment variable. ` +
`If you add the "${APIFY_ENV_VARS.TOKEN}" environment variable, the password will be automatically inferred.`,
`If you add the "${APIFY_ENV_VARS.TOKEN}" environment variable, ` +
`while not providing "${APIFY_ENV_VARS.PROXY_PASSWORD}", the password will be automatically inferred.`,
);
}
}
Expand Down
20 changes: 5 additions & 15 deletions test/apify/proxy_configuration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,26 +619,16 @@ describe('Actor.createProxyConfiguration()', () => {
getUserSpy.mockRestore();
});

test('should show warning log', async () => {
process.env.APIFY_TOKEN = '123456789';
test(`shouldn't request password from API when both PROXY_PASSWORD and TOKEN envs are provided`, async () => {
process.env[APIFY_ENV_VARS.TOKEN] = 'some_token';
process.env[APIFY_ENV_VARS.PROXY_PASSWORD] = 'proxy_password';

const getUserSpy = vitest.spyOn(UserClient.prototype, 'get');
const status = { connected: true };
const fakeUserData = {
proxy: { password: 'some-other-users-password' },
};
getUserSpy.mockResolvedValueOnce(fakeUserData as any);
gotScrapingSpy.mockResolvedValueOnce({ body: status } as any);

const proxyConfiguration = new ProxyConfiguration(basicOpts);
// @ts-expect-error
const logMock = vitest.spyOn(proxyConfiguration.log, 'warning');
const proxyConfiguration = new ProxyConfiguration();
await proxyConfiguration.initialize();
expect(logMock).toBeCalledTimes(1);
expect(getUserSpy).toBeCalledTimes(0);

logMock.mockRestore();
getUserSpy.mockRestore();
gotScrapingSpy.mockRestore();
});

// TODO: test that on platform we throw but locally we only print warning
Expand Down
Loading