Unencrypted support connections plus proxy support#32
Unencrypted support connections plus proxy support#32EionRobb merged 8 commits intoEionRobb:masterfrom
Conversation
Thaodan
force-pushed
the
thaodan/connection_improvements
branch
4 times, most recently
from
2810565 to
f3798c1
Compare
|
Everything works now except that in Bitlbee the select encryption option somehow doesn't work, have to check. |
Thaodan
force-pushed
the
thaodan/connection_improvements
branch
2 times, most recently
from
1010377 to
e67378f
Compare
|
Works now in Bitlbee too, Bitlbee didn't like the reverse option list. |
|
Might also want to check that all the tabs didn't turn into spaces before you mark it as ready to merge :) |
Thaodan
force-pushed
the
thaodan/connection_improvements
branch
from
e67378f to
1dfbaf3
Compare
Fixing that right now. Are you open to adding editorconfig to the repo to avoid such issues in the future? |
Thaodan
changed the title
Sounds good :) |
Thaodan
force-pushed
the
thaodan/connection_improvements
branch
from
b240888 to
463b195
Compare
|
|
||
|
|
||
| if (ya->websocket_server != ya->server) { | ||
| host = g_strdup(ya->websocket_server); |
There was a problem hiding this comment.
Does this leak the memory of the old host? Need g_free(host) before assign?
There was a problem hiding this comment.
Do I need to do so? host is just a plain char array without malloc created in the purple function.
librocketchat.c
Outdated
| if (ya->websocket != NULL) | ||
| purple_ssl_close(ya->websocket); | ||
| else if (ya->fd > 0) { | ||
| if (ya->pc->inpa) |
There was a problem hiding this comment.
is pc->inpa exposed through a function rather than direct struct access? If so can we use that func for forwards compatibility?
There was a problem hiding this comment.
Purples Jabber plugin also uses the same way to access pc. The pc is part of the account structure, I'm not sure how that would change with Purple 3.x but I don't see any of that so far.
There was a problem hiding this comment.
Purple 3 makes all the eg PurpleConnection and PurpleAccount structs private
There was a problem hiding this comment.
Oh it seams I should have used ya->inpa instead.
Purple 3.x would do g_source_remove(ya->inpa).
There was a problem hiding this comment.
I kept that part as Purple 2.x does it the same for Jabber.
The Purple 3.x section would look differently anyway..
The plugin "uses" purple_ssl_connect for Purple 3.x however this has been removed and replaced by Gio.
E.g. see here:
https://keep.imfreedom.org/pidgin/pidgin/file/tip/libpurple/protocols/jabber/jabber.c#l686
I would favor to merge this section as is and the Purple 3.x support should be fixed in another PR (#33).
| "connection_security", encryption_values); | ||
| account_options = g_list_append(account_options, option); | ||
|
|
||
| option = purple_account_option_string_new(N_("Proxy"), "proxy", ""); |
There was a problem hiding this comment.
There's proxy options available in the libpurple api that can be used instead from the proxy tab using the PurpleProxyInfo stuff in purple_proxy_get_setup() https://docs.imfreedom.org/pidgin2/proxy_8h.html#a6995e6f63718dcd8644b9d4ef471392a if that makes things easier?
There was a problem hiding this comment.
The thing is that isn't really a "proxy", it's more like using another host instead of the real host.
It works by using either stunnel or socat as an ssl proxy - both work fine from my tests.
I'm not sure, does using purple_proxy_get_setup() set anything else?
There was a problem hiding this comment.
Oh right! This is so you can add in the client cert? Just putting 2 and 2 together now. With a label like "proxy" I wouldn't want anyone to get tripped up with the other Proxy tab.
Naming stuff is hard.
What about "intermediate server"?
There was a problem hiding this comment.
What about "intermediate server"?
Isn't that also a proxy? I think proxy is accurate.
There was a problem hiding this comment.
Oh right! This is so you can add in the client cert? Just putting 2 and 2 together now. With a label like "proxy" I wouldn't want anyone to get tripped up with the other Proxy tab.
Yes this and the unencrypted connection so there's no SSL twice.
|
Awesome stuff! |
- Always initialize variables, ping_frame_len was uninitialized in some cases. - Remove redundant else after if websocket closed switch to goto try_reconnect section. - Handle also errno EINPROGRESS and ENOENT - Indent Signed-off-by: Björn Bidar <bjorn.bidar@thaodan.de>
- Abstract all operations that affect sockets into small wrappers - Initialize either TLS or proxy connection depending on the setting Signed-off-by: Björn Bidar <bjorn.bidar@thaodan.de>
The option allows the use of a proxy server that is used instead of the real server. Since the real server is still needed inside `rc_socket_upgrade()` we still have to keep it for that. Signed-off-by: Björn Bidar <bjorn.bidar@thaodan.de>
Read below for more: https://editorconfig.org/ Signed-off-by: Björn Bidar <bjorn.bidar@thaodan.de>
g_memdump is deprecated in glib 2.68.0. Signed-off-by: Björn Bidar <bjorn.bidar@thaodan.de>
Signed-off-by: Björn Bidar <bjorn.bidar@thaodan.de>
Thaodan
force-pushed
the
thaodan/connection_improvements
branch
from
df283d9 to
b889eb7
Compare
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Plugin
participant Server
User->>Plugin: Configure account (set TLS/Non-TLS, Proxy)
Plugin->>Plugin: Read "connection_security" and "proxy" options
Plugin->>Server: Establish connection (TLS or Non-TLS)
alt TLS
Plugin->>Server: Connect via purple_ssl_connect
else Non-TLS
Plugin->>Server: Connect via purple_proxy_connect
end
Plugin->>Server: Send websocket upgrade request
Server-->>Plugin: Websocket upgrade response
Plugin->>Plugin: Use rc_sock_read/write for socket I/O
Plugin->>Server: HTTP/Websocket communication (with correct Host and URL)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 Gitleaks (8.27.2)librocketchat.c2697-2697: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (14)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
librocketchat.c (5)
27-27: Fix typo:g_memdump2should beg_memdup2
840-840: Use string comparison instead of pointer comparison
881-881: Use string comparison for proxy check
881-882: Free the original host value to prevent memory leakThe
hostvariable allocated bypurple_url_parseneeds to be freed before reassignment.if (!purple_strequal(ya->websocket_server, ya->server)) { + g_free(host); host = g_strdup(ya->websocket_server);
2279-2280: Use ya->inpa instead of pc->inpaAccording to the
RocketChatAccountstructure definition,inpais a field ofya, notpc.-if (ya->pc->inpa) - purple_input_remove(ya->pc->inpa); +if (ya->inpa) + purple_input_remove(ya->inpa);
🧹 Nitpick comments (2)
.editorconfig (2)
4-4: Prefer LF line endings for cross-platform friendlinessMost POSIX tooling and Git defaults assume LF. Enforcing
crlfoften leads to noisy diffs and inconsistent behaviour on non-Windows systems. Unless the entire codebase already uses CRLF, switch to LF.-end_of_line = crlf +end_of_line = lf
3-3: Specifyindent_sizealongsideindent_styleWithout an explicit
indent_size, editors fall back to their default tab width (commonly 8). Defining it (e.g. 4) ensures uniform visual alignment across contributors.indent_style = tab +indent_size = 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.editorconfig(1 hunks)librocketchat.c(23 hunks)
🧰 Additional context used
🪛 Gitleaks (8.27.2)
librocketchat.c
2694-2694: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (14)
librocketchat.c (14)
255-274: LGTM! Structure fields properly support dual connection modesThe new fields appropriately support both SSL and plain socket connections, with clear separation of concerns between the websocket server and proxy configuration.
1034-1038: LGTM! URL construction properly handles both HTTP and HTTPSThe dynamic protocol prefix based on connection security settings is implemented correctly.
1473-1478: LGTM! Attachment URLs correctly use protocol prefix
2191-2209: LGTM! Connection security and proxy setup implemented correctlyThe code properly initializes TLS settings and HTTP protocol strings based on user configuration, and handles proxy settings appropriately.
2236-2238: LGTM! Login URL uses correct protocol prefix
2322-2324: LGTM! Proper cleanup of new string fields
2484-2485: LGTM! Proper connection validation for both modes
2524-2558: LGTM! Consistent use of socket read abstractionAll socket read operations properly use the new
rc_sock_readabstraction.
2559-2673: LGTM! Comprehensive error handling for dual connection modesThe error handling properly distinguishes between different failure scenarios and provides appropriate error messages.
2676-2760: LGTM! Clean separation of SSL and proxy callbacksThe callback functions provide a clean abstraction layer for handling both connection types.
2783-2820: LGTM! Proper dual-mode connection initializationThe code correctly handles both SSL and proxy connections with appropriate error handling.
2742-2743: LGTM! Avatar URL uses correct protocol
3793-3831: LGTM! Account options properly support new featuresThe connection security dropdown and proxy settings are implemented correctly with clear options for users.
4088-4089: LGTM! Consistent plugin descriptions
|
Sorry that it took soooo loooong to get this merged in |
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes