mirror of
https://github.com/NginxProxyManager/nginx-proxy-manager.git
synced 2026-11-15 16:38:04 +00:00
Fix second round of review findings from PR #6029
Security - Check advanced_config permissions before creating a certificate on host update (proxy, redirection and 404 hosts). A refused request previously requested a Let's Encrypt certificate first, leaving an orphan certificate and using up rate limit. - Only trust X-NPM-Peer-Addr when it comes with a secret generated at container start. The admin nginx sends both headers. Any proxy host or stream forwarding to 127.0.0.1:3000 was also a loopback peer, so it could forge the header and get a fresh rate-limit bucket on every request. Without the secret (dev, or a mounted production.conf), the header is ignored. - Add a login limit per IP across all accounts (30 failed attempts a minute) alongside the per-account one. Before this, one address could spray passwords over any number of accounts. Behind a shared proxy, everyone using that proxy shares this limit. - Stop trusting X-Real-IP from loopback. It was only there for an older version of the rate limiter, and it let a stream host forwarding to a local proxy host forge the client IP and get past IP access lists. Loopback now behaves as it does on master. Correctness - Refuse to delete a certificate that hosts still use (400, with counts per host type). Their configs include its files, and a successful revoke deleted them anyway, which broke nginx -t. Otherwise the certificate is revoked and its files are removed even if the revoke fails. - Let non-admins clear a location's advanced config. Deleting the location was already allowed and has the same effect, and a location deleted and re-added at the same path arrives with an empty config, which was refused. - Clear proxy host grants (meta.proxy_host_ids) when visibility isn't "user" or proxy hosts are hidden, and drop ids of deleted hosts. Before, hidden grants came back when proxy hosts were shown again. The backend enforces this; the permissions modal also stops sending them. - 404 host and redirection host modals now wait for the current user to load, so an admin's advanced config change isn't dropped if they submit before it loads. Tests - NonAdminHosts.cy.js: refused change doesn't create a certificate, location rename and clear rules, grant clearing. - Certificates.cy.js: deleting a certificate that is in use.
This commit is contained in:
1 parent
04c1f5ae76
commit
5b7e34eb67
18 files changed
+423
-62
No files matched your search
@@ -143,6 +143,79 @@ describe('Certificates endpoints', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('Certificate in use by a host cannot be deleted', () => {
|
||||
cy.task('backendApiPost', {
|
||||
token: token,
|
||||
path: '/api/nginx/certificates',
|
||||
data: {
|
||||
provider: 'other',
|
||||
nice_name: 'In Use Certificate',
|
||||
},
|
||||
}).then((cert) => {
|
||||
cy.task('backendApiPostFiles', {
|
||||
token: token,
|
||||
path: `/api/nginx/certificates/${cert.id}/upload`,
|
||||
files: {
|
||||
certificate: certFile,
|
||||
certificate_key: keyFile,
|
||||
},
|
||||
}).then(() => {
|
||||
cy.task('backendApiPost', {
|
||||
token: token,
|
||||
path: '/api/nginx/proxy-hosts',
|
||||
data: {
|
||||
domain_names: ['cert-in-use.example.com'],
|
||||
forward_scheme: 'http',
|
||||
forward_host: '1.1.1.1',
|
||||
forward_port: 80,
|
||||
access_list_id: '0',
|
||||
certificate_id: cert.id,
|
||||
meta: { dns_challenge: false },
|
||||
advanced_config: '',
|
||||
locations: [],
|
||||
block_exploits: false,
|
||||
caching_enabled: false,
|
||||
allow_websocket_upgrade: false,
|
||||
http2_support: false,
|
||||
hsts_enabled: false,
|
||||
hsts_subdomains: false,
|
||||
ssl_forced: false,
|
||||
},
|
||||
}).then((host) => {
|
||||
cy.task('backendApiDelete', {
|
||||
token: token,
|
||||
path: `/api/nginx/certificates/${cert.id}`,
|
||||
returnOnError: true,
|
||||
}).then((data) => {
|
||||
cy.validateSwaggerSchema('delete', 400, '/nginx/certificates/{certID}', data);
|
||||
expect(data.error.code).to.equal(400);
|
||||
expect(data.error.message).to.contain('in use by 1 proxy host');
|
||||
});
|
||||
|
||||
// Still there
|
||||
cy.task('backendApiGet', {
|
||||
token: token,
|
||||
path: `/api/nginx/certificates/${cert.id}`,
|
||||
}).then((data) => {
|
||||
expect(data.id).to.equal(cert.id);
|
||||
});
|
||||
|
||||
// Once the host no longer uses it, it can be deleted
|
||||
cy.task('backendApiDelete', {
|
||||
token: token,
|
||||
path: `/api/nginx/proxy-hosts/${host.id}`,
|
||||
});
|
||||
cy.task('backendApiDelete', {
|
||||
token: token,
|
||||
path: `/api/nginx/certificates/${cert.id}`,
|
||||
}).then((data) => {
|
||||
expect(data).to.be.equal(true);
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
it('Request Certificate - CVE-2024-46256/CVE-2024-46257', () => {
|
||||
cy.task('backendApiPost', {
|
||||
token: token,
|
||||
|
||||
@@ -0,0 +1,184 @@
|
||||
/// <reference types="cypress" />
|
||||
|
||||
describe('Non-admin host and permission rules', () => {
|
||||
const ADMIN_CONFIG = 'add_header X-Admin-Only "1";';
|
||||
const nonAdmin = {
|
||||
email: 'nonadmin-hosts@example.com',
|
||||
secret: 'changeme',
|
||||
};
|
||||
|
||||
let token;
|
||||
let userId;
|
||||
let userToken;
|
||||
let hostId;
|
||||
|
||||
const hostPayload = (overrides) => Object.assign({
|
||||
domain_names: ['nonadmin-hosts.example.com'],
|
||||
forward_scheme: 'http',
|
||||
forward_host: '1.1.1.1',
|
||||
forward_port: 80,
|
||||
access_list_id: '0',
|
||||
certificate_id: 0,
|
||||
meta: { dns_challenge: false },
|
||||
advanced_config: '',
|
||||
locations: [],
|
||||
block_exploits: false,
|
||||
caching_enabled: false,
|
||||
allow_websocket_upgrade: false,
|
||||
http2_support: false,
|
||||
hsts_enabled: false,
|
||||
hsts_subdomains: false,
|
||||
ssl_forced: false,
|
||||
}, overrides || {});
|
||||
|
||||
const location = (path, advancedConfig) => ({
|
||||
path: path,
|
||||
forward_scheme: 'http',
|
||||
forward_host: '1.1.1.1',
|
||||
forward_port: 8080,
|
||||
advanced_config: advancedConfig,
|
||||
});
|
||||
|
||||
const getPermissions = () => cy.task('backendApiGet', {
|
||||
token: token,
|
||||
path: `/api/users/${userId}?expand=permissions`,
|
||||
}).then((user) => user.permissions);
|
||||
|
||||
before(() => {
|
||||
cy.resetUsers();
|
||||
cy.getToken().then((tok) => {
|
||||
token = tok;
|
||||
|
||||
// Admin-owned host with an admin-written location config
|
||||
cy.task('backendApiPost', {
|
||||
token: token,
|
||||
path: '/api/nginx/proxy-hosts',
|
||||
data: hostPayload({ locations: [location('/api', ADMIN_CONFIG)] }),
|
||||
}).then((host) => {
|
||||
hostId = host.id;
|
||||
|
||||
cy.task('backendApiPost', {
|
||||
token: token,
|
||||
path: '/api/users',
|
||||
data: {
|
||||
name: 'Non Admin',
|
||||
nickname: 'NonAdmin',
|
||||
email: nonAdmin.email,
|
||||
roles: [],
|
||||
auth: { type: 'password', secret: nonAdmin.secret },
|
||||
},
|
||||
}).then((user) => {
|
||||
userId = user.id;
|
||||
cy.task('backendApiPut', {
|
||||
token: token,
|
||||
path: `/api/users/${userId}/permissions`,
|
||||
data: {
|
||||
visibility: 'user',
|
||||
proxy_hosts: 'manage',
|
||||
certificates: 'manage',
|
||||
meta: { proxy_host_ids: [hostId] },
|
||||
},
|
||||
});
|
||||
cy.getToken(null, { identity: nonAdmin.email, secret: nonAdmin.secret }).then((tok2) => {
|
||||
userToken = tok2;
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
it('Refused advanced config change does not request a certificate first', () => {
|
||||
cy.task('backendApiGet', { token: token, path: '/api/nginx/certificates' }).then((before) => {
|
||||
cy.task('backendApiPut', {
|
||||
token: userToken,
|
||||
path: `/api/nginx/proxy-hosts/${hostId}`,
|
||||
data: { certificate_id: 'new', advanced_config: 'return 418;' },
|
||||
returnOnError: true,
|
||||
}).then((data) => {
|
||||
expect(data).to.have.property('error');
|
||||
expect(data.error.code).to.equal(403);
|
||||
cy.task('backendApiGet', { token: token, path: '/api/nginx/certificates' }).then((after) => {
|
||||
expect(after.length).to.equal(before.length);
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
it('Non-admin cannot add new location advanced config', () => {
|
||||
cy.task('backendApiPut', {
|
||||
token: userToken,
|
||||
path: `/api/nginx/proxy-hosts/${hostId}`,
|
||||
data: { locations: [location('/api', ADMIN_CONFIG), location('/new', 'return 418;')] },
|
||||
returnOnError: true,
|
||||
}).then((data) => {
|
||||
expect(data).to.have.property('error');
|
||||
expect(data.error.code).to.equal(403);
|
||||
});
|
||||
});
|
||||
|
||||
it('Non-admin can rename a location and keep its admin config', () => {
|
||||
cy.task('backendApiPut', {
|
||||
token: userToken,
|
||||
path: `/api/nginx/proxy-hosts/${hostId}`,
|
||||
data: { locations: [location('/v2', ADMIN_CONFIG), location('/api', '')] },
|
||||
}).then((data) => {
|
||||
const byPath = Object.fromEntries(data.locations.map((l) => [l.path, l.advanced_config]));
|
||||
expect(byPath['/v2']).to.equal(ADMIN_CONFIG);
|
||||
expect(byPath['/api']).to.equal('');
|
||||
});
|
||||
});
|
||||
|
||||
it('Non-admin can delete and re-add a location at the same path', () => {
|
||||
// The location form sends advanced_config "" for a new row
|
||||
cy.task('backendApiPut', {
|
||||
token: userToken,
|
||||
path: `/api/nginx/proxy-hosts/${hostId}`,
|
||||
data: { locations: [location('/v2', '')] },
|
||||
}).then((data) => {
|
||||
expect(data.locations).to.have.length(1);
|
||||
expect(data.locations[0].advanced_config).to.equal('');
|
||||
});
|
||||
});
|
||||
|
||||
it('Proxy host grants are cleared when proxy hosts are hidden', () => {
|
||||
// Unknown ids are dropped
|
||||
cy.task('backendApiPut', {
|
||||
token: token,
|
||||
path: `/api/users/${userId}/permissions`,
|
||||
data: { proxy_hosts: 'manage', meta: { proxy_host_ids: [hostId, 999999] } },
|
||||
});
|
||||
getPermissions().then((perms) => {
|
||||
expect(perms.meta.proxy_host_ids).to.deep.equal([hostId]);
|
||||
});
|
||||
|
||||
cy.task('backendApiPut', {
|
||||
token: token,
|
||||
path: `/api/users/${userId}/permissions`,
|
||||
data: { proxy_hosts: 'hidden' },
|
||||
});
|
||||
getPermissions().then((perms) => {
|
||||
expect(perms.meta.proxy_host_ids).to.deep.equal([]);
|
||||
});
|
||||
|
||||
// Showing proxy hosts again does not bring the old grant back
|
||||
cy.task('backendApiPut', {
|
||||
token: token,
|
||||
path: `/api/users/${userId}/permissions`,
|
||||
data: { proxy_hosts: 'manage' },
|
||||
});
|
||||
getPermissions().then((perms) => {
|
||||
expect(perms.meta.proxy_host_ids).to.deep.equal([]);
|
||||
});
|
||||
});
|
||||
|
||||
it('Proxy host grants are cleared when visibility is all', () => {
|
||||
cy.task('backendApiPut', {
|
||||
token: token,
|
||||
path: `/api/users/${userId}/permissions`,
|
||||
data: { visibility: 'all', proxy_hosts: 'manage', meta: { proxy_host_ids: [hostId] } },
|
||||
});
|
||||
getPermissions().then((perms) => {
|
||||
expect(perms.meta.proxy_host_ids).to.deep.equal([]);
|
||||
});
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user